Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

run go mod tidy #8402

Merged
merged 3 commits into from
Nov 29, 2018
Merged

run go mod tidy #8402

merged 3 commits into from
Nov 29, 2018

Conversation

gregwebs
Copy link
Contributor

@gregwebs gregwebs commented Nov 22, 2018

There is a lot of junk in go.sum right now. This is because as I understand it, go.sum is only pruned by go mod tidy.
Additionally, we are missing indirect dependencies, which may hamper reproducible builds.

We should add go mod tidy to our CI process.


This change is Reviewable

@zz-jason
Copy link
Member

should we add go mod tidy to the check section in the Makefile?

@tiancaiamao
Copy link
Contributor

I'm fine with the changes, but I'm not sure about the rule:
Should we run go mod tidy every time before pushing the code ?
Or after a GO111MODULE=on go get -u operation ?
Or each time we release a new version ?

What do you think @gregwebs

@gregwebs
Copy link
Contributor Author

I would prefer to keep it tidy because I like to be able to see transitive dependencies. It is always possible to get the older go.sum from version control. So to verify that it has been run doing it as a check makes sense.
Once it has been tidied, I think it should only change after changing dependencies, so the workflow is to run it after a dependency change.

@gregwebs
Copy link
Contributor Author

oh, adding it as a make check will make for extra effort because the files may already be changed in the working copy. Checking on CI will be the easiest (run go mod tidy and then see if there are dirty files).

@tiancaiamao
Copy link
Contributor

So, would you like to add this rule in the Makefile, just like the gofmt rule:

tidy:
	@echo "go mod tidy"
	GO111MODULE=on go mod tidy 2>&1 | $(FAIL_ON_STDOUT)

@gregwebs
Copy link
Contributor Author

Checking stdout/stderr is brittle, particularly for a feature labeled as experimental. On the CI I would run go mod tidy and then run git diff --quiet (should have a zero exit status).

@tiancaiamao
Copy link
Contributor

go mod tidy and then run git diff --quiet is also acceptable, add it to Makefile and update this PR
@gregwebs

@gregwebs gregwebs force-pushed the go-mod-tidy branch 2 times, most recently from de2ec4a to c59c710 Compare November 28, 2018 22:24
@gregwebs
Copy link
Contributor Author

okay, added. Ironically go.sum is changing daily and keeps conflicting with this branch (precisely what this PR will address).

@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 29, 2018
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/server status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants