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

Ensure dep has been run in CI #755

Merged
merged 1 commit into from
Jun 18, 2019
Merged

Ensure dep has been run in CI #755

merged 1 commit into from
Jun 18, 2019

Conversation

johnSchnake
Copy link
Contributor

What this PR does / why we need it:
If someone fails to add a dep then the build will fail but
if they remove one and dont run it then it can lead to
deps hanging around unnecessarily and a future dev/PR getting
a surprising diff.

Which issue(s) this PR fixes
Fixes #244

Special notes for your reviewer:
dep says that the command is supposed to exit non-zero if it would write something but that is just not true if you try it out/look at the code. Just checking that something gets written to stdout which seems (by looking into the code) to the only way to tell.

I am trying not to hinge on an exact wording like Would have written just in case that gets tweaked at some point. We are also pinning the version here though so it should be fine.

Release note:

NONE

@johnSchnake johnSchnake requested a review from zubron June 18, 2019 17:00
@codecov-io
Copy link

codecov-io commented Jun 18, 2019

Codecov Report

Merging #755 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #755      +/-   ##
==========================================
- Coverage   42.59%   42.54%   -0.05%     
==========================================
  Files          70       70              
  Lines        4029     4029              
==========================================
- Hits         1716     1714       -2     
- Misses       2208     2210       +2     
  Partials      105      105
Impacted Files Coverage Δ
cmd/sonobuoy/app/status.go 57.14% <0%> (-2.2%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 985c071...f2d0448. Read the comment docs.

@johnSchnake
Copy link
Contributor Author

./dep ensure --dry-run produced output on stdout; did you need add/remove a dependency?

But didnt fail the build; forgot to exit 1

However, there isn't a reason the lockfile should be written; it is the same data. Didnt think this was the case the first time I tried it but /shrug. May need to check for some other value besides the lockfile. Will double check the code but maybe just an inverted grep is enough?

If someone fails to add a dep then the build will fail but
if they remove one and dont run it then it can lead to
deps hanging around unnecessarily and a future dev/PR getting
a surprising diff.

Fixes #244

Signed-off-by: John Schnake <[email protected]>
@johnSchnake
Copy link
Contributor Author

Pushed a commit to show that section now fails as expected. Has since removed it (but you can look at the travis history if you want to see the failure).

I ended up setting the failure against an actual dep ensure && git status check because (1) it is clear what/why we are checking (2) dep ensure --dry-run oddly always wrote that it was going to write the gopkg.lock so we would have had to do a more awkward, unintuitive check.

Copy link
Contributor

@zubron zubron left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@johnSchnake johnSchnake merged commit 8ac5ca2 into vmware-tanzu:master Jun 18, 2019
@johnSchnake johnSchnake deleted the depEnsureCI branch June 18, 2019 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI check that dep ensure has been run
3 participants