Skip to content

hack: replace go-mod-prepare.sh with wrapper script#44497

Merged
thaJeztah merged 2 commits intomoby:masterfrom
neersighted:no_uncommitted_go_mod
Dec 13, 2022
Merged

hack: replace go-mod-prepare.sh with wrapper script#44497
thaJeztah merged 2 commits intomoby:masterfrom
neersighted:no_uncommitted_go_mod

Conversation

@neersighted
Copy link
Copy Markdown
Member

@neersighted neersighted commented Nov 18, 2022

- What I did
To make the local build environment more correct and consistent, we should never leave an uncommitted go.mod in the tree; however, we need a go.mod for certain commands to work properly. Use a wrapper script to create and destroy the go.mod as needed instead of potentially changing tooling behavior by leaving it.

Also add a script to prevent go.mod from sneaking back in; as we are not a Go module, and cannot currently become one, we want to prevent new contributors from mistakenly trying to turn Moby into one.

- How to verify it
Manually, more or less.

- A picture of a cute animal (not mandatory but encouraged)
image

Comment thread hack/with-go-mod.sh Outdated
Comment thread hack/vendor.sh Outdated
Comment thread .gitignore Outdated
@neersighted neersighted force-pushed the no_uncommitted_go_mod branch from fda3346 to c77cc24 Compare November 18, 2022 22:58
@neersighted
Copy link
Copy Markdown
Member Author

Pushed up a new version that adds a new validate step to go with the removal of go.mod from .gitignore.

@thaJeztah
Copy link
Copy Markdown
Member

@neersighted needs a rebase

@neersighted
Copy link
Copy Markdown
Member Author

This is stacked on #44459; do you want to land that and I'll rebase this on top of the merged branch?

@thaJeztah
Copy link
Copy Markdown
Member

This is stacked on #44459

Oh! My bad; I'll have a look at that one.

@thaJeztah
Copy link
Copy Markdown
Member

The other one was merged; so this one can be rebased 👍

@neersighted neersighted force-pushed the no_uncommitted_go_mod branch from 9710ee2 to 3082be9 Compare December 12, 2022 19:07
@neersighted
Copy link
Copy Markdown
Member Author

Rebased 👍

@thaJeztah
Copy link
Copy Markdown
Member

Looks like shfmt isn't happy 🙈

To make the local build environment more correct and consistent, we
should never leave an uncommitted go.mod in the tree; however, we need a
go.mod for certain commands to work properly. Use a wrapper script to
create and destroy the go.mod as needed instead of potentially changing
tooling behavior by leaving it.

If a go.mod already exists, this script will warn and call the wrapped
command with GO111MODULE=on.

Signed-off-by: Bjorn Neergaard <[email protected]>
Moby is not a Go module; to prevent anyone from mistakenly trying to
convert it to one before we are ready, introduce a check (usable in CI
and locally) for a go.mod file.

This is preferable to trying to .gitignore the file as we can ensure
that a mistakenly created go.mod is surfaced by Git-based tooling and is
less likely to surprise a contributor.

Signed-off-by: Bjorn Neergaard <[email protected]>
@neersighted neersighted force-pushed the no_uncommitted_go_mod branch from 3082be9 to 25c3421 Compare December 13, 2022 01:39
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread .gitignore
Comment on lines -16 to -17
# top-level go.mod is not meant to be checked in
/go.mod
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still slightly on the fence if this could cause accidentally adding the go.mod (if the script failed at some point etc), but I'll guess we'll see how it goes 🙈

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hack/validate/no-module added in this PR would catch those cases. Though it will need to be added to hack/validate/default to be run in CI?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I excluded it from default because some users may want to be able to have the tooling function with a go.mod present but not committed. It's run in CI as there is a file-based glob. However, we could add it to all (the same step where vendoring is run).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's run in CI as there is a file-based glob.

@neersighted I don't see it (neither in the current git HEAD, nor in the source tree during the state when this PR got merged.

@thaJeztah
Copy link
Copy Markdown
Member

@neersighted could you prepare a 🍒 ⛏️ for this one (and the previous PR)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants