CI: Make CI not update the lock file #2152

ValarDragon · 2018-08-25T23:47:53Z

We want CI to be running the lock in the repo, not generating a new one.
Linting ensures that the lock file is up to date, so thats not a concern here.

I hope that this will prevent the upload_coverage error in https://circleci.com/gh/cosmos/cosmos-sdk/24023 from happening again.
I believe we discussed doing this previously @cwgoes.

Targeted PR against correct branch (see CONTRIBUTING.md)
Added entries in PENDING.md with issue # - I don't think CI changes are relevant to anyone reading our changelog

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

We want CI to be running the lock in the repo, not generating a new one. Linting now ensures that the lock file is up to date.

codecov · 2018-08-25T23:52:41Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@5128a7c). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop    #2152   +/-   ##
==========================================
  Coverage           ?   63.91%           
==========================================
  Files              ?      134           
  Lines              ?     8194           
  Branches           ?        0           
==========================================
  Hits               ?     5237           
  Misses             ?     2604           
  Partials           ?      353

alexanderbez

Thanks @ValarDragon ! Just left a small remark 👍

alexanderbez · 2018-08-26T03:11:12Z

Makefile

@@ -110,6 +110,11 @@ get_vendor_deps:
 	@rm -rf .vendor-new
 	@dep ensure -v

+get_vendor_deps_ci:


Almost an identical target to get_vendor_deps right? Why not just add a build env var/flag (i.e. @dep ensure -v $(VENDOR)) to the existing target (where the default value is false)?

I'm not sure. I think makefiles tend to be super hard to read, so I prefer not using flags for the simple things in the main commands.

I don't feel strongly about this, so if you'd prefer flags id be happy to change it

Nor do I -- you can keep as is.

melekes · 2018-08-27T09:00:55Z

.circleci/config.yml

@@ -207,7 +207,7 @@ jobs:
            command: |
              set -x
              make get_tools
-              make get_vendor_deps
+              make get_vendor_deps_ci


I remember we had something like that in Tendermint, but switched to make get_vendor_deps. Does this mean we should do the same change in Tendermint?

Probably. We made that change before we ensured the lock file is correct, but now that we do, we can go back to how we used to do it Tendermint!

alexanderbez

LGTM 👌

cwgoes

I'm not convinced having CI and the default make command do different things is a good idea. I agree that we should use the existing lockfile on CI - but why not also use it locally, and require an explicit make update_vendor_deps or something to update it?

That would also make an audit of a particular hash-locked version set of our dependency tree more meaningful, since we could warn users when they pull updated (therefore unaudited) versions.

alexanderbez · 2018-08-27T17:26:29Z

I think this is a safer approach @cwgoes. Also less noise in PRs.

ValarDragon · 2018-08-27T17:36:38Z

I agree. However we do lock revisions, so we already have that aspect

rigelrozanski

I think it's okay if CI generates a new one - the point is to stick with whatever go dep wants to do - if something is failing because of a dep update in CI we should go and fix it before we merge that PR. What we want to avoid at all costs is divergent "custom" deps which are not aligned with go dep this is what happened previously when we were using glide and it was a huge headache. Many many wasted engineering hours in dep hell.

ValarDragon · 2018-08-27T17:56:24Z

I don't really understand @rigelrozanski, this PR is fixing the situation where CI diverges from what is in the repo. Now it will only run based off what the lock file is actualyl saying, why is this an undesirable property? If we have people use get_vendor_deps and that never updates the lock file, then this should alleviate your concern entirely.

I agree with changing the semantics and usage everywhere to get_vendor_deps and update_vendor_deps though.

rigelrozanski · 2018-08-27T18:01:51Z

Looks good based on current change to distinguish update and get - ALTHOUGH we must now use extreme caution to not manually edit the lock file to avoid previous dep hell situation.

Edit:

why is this an undesirable property?

By not forcing us to update deps constantly (or not use versioning properly) I think we could create a situation like we've previously seen where folks will end up just editing the lock file, making in the long term a proper dep-update an arduous situation for a single dev to undertake.

neutral

cwgoes · 2018-08-28T11:48:04Z

By not forcing us to update deps constantly (or not use versioning properly) I think we could create a situation like we've previously seen where folks will end up just editing the lock file, making in the long term a proper dep-update an arduous situation for a single dev to undertake.

If you want to update dependencies, run make update_vendor_deps. If you want to pin a specific version, edit Gopkg.toml (like we do already sometimes). What is the situation in which you would need to manually edit the lockfile?

cwgoes

utACK but waiting for @rigelrozanski

rigelrozanski · 2018-08-28T16:53:34Z

it's a hack move, previously with glide people would want to keep the branch reference for clarity in the yaml while simultaneously, locking the version used to a particular hash on that branch within the lock file. Maybe this is now an obsolete concern of mine as it looks like almost everything is referenced semanticly...... good to merge, I'll just get my ghosts to haunt anyone who dares attempt to edit the lock file directly

CI: Make CI not update the lock file

d6a0f63

We want CI to be running the lock in the repo, not generating a new one. Linting now ensures that the lock file is up to date.

ValarDragon assigned cwgoes Aug 25, 2018

ValarDragon requested a review from alexanderbez August 25, 2018 23:47

ValarDragon requested review from cwgoes, ebuchman and rigelrozanski as code owners August 25, 2018 23:47

ValarDragon added ready-for-review labels Aug 25, 2018

alexanderbez reviewed Aug 26, 2018

View reviewed changes

melekes approved these changes Aug 27, 2018

View reviewed changes

alexanderbez approved these changes Aug 27, 2018

View reviewed changes

cwgoes suggested changes Aug 27, 2018

View reviewed changes

rigelrozanski suggested changes Aug 27, 2018

View reviewed changes

Switch to Chris' comment

a65742d

rigelrozanski previously approved these changes Aug 27, 2018

View reviewed changes

Update pending to indicate this new command

4c7441a

cwgoes approved these changes Aug 28, 2018

View reviewed changes

rigelrozanski merged commit 946e24d into develop Aug 28, 2018

rigelrozanski deleted the dev/ci_lockfile branch August 28, 2018 16:54

abelliumnt mentioned this pull request Sep 21, 2018

R4R: add deps tools, add update_vendor_deps irisnet/irishub#283

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: Make CI not update the lock file #2152

CI: Make CI not update the lock file #2152

ValarDragon commented Aug 25, 2018 •

edited

Loading

codecov bot commented Aug 25, 2018

alexanderbez left a comment

alexanderbez Aug 26, 2018 •

edited

Loading

ValarDragon Aug 26, 2018

alexanderbez Aug 27, 2018

melekes Aug 27, 2018

ValarDragon Aug 27, 2018 •

edited

Loading

alexanderbez left a comment

cwgoes left a comment •

edited

Loading

alexanderbez commented Aug 27, 2018

ValarDragon commented Aug 27, 2018

rigelrozanski left a comment •

edited

Loading

ValarDragon commented Aug 27, 2018 •

edited

Loading

rigelrozanski commented Aug 27, 2018 •

edited

Loading

cwgoes commented Aug 28, 2018

cwgoes left a comment

rigelrozanski commented Aug 28, 2018

CI: Make CI not update the lock file #2152

CI: Make CI not update the lock file #2152

Conversation

ValarDragon commented Aug 25, 2018 • edited Loading

codecov bot commented Aug 25, 2018

Codecov Report

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez Aug 26, 2018 • edited Loading

Choose a reason for hiding this comment

ValarDragon Aug 26, 2018

Choose a reason for hiding this comment

alexanderbez Aug 27, 2018

Choose a reason for hiding this comment

melekes Aug 27, 2018

Choose a reason for hiding this comment

ValarDragon Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

cwgoes left a comment • edited Loading

Choose a reason for hiding this comment

alexanderbez commented Aug 27, 2018

ValarDragon commented Aug 27, 2018

rigelrozanski left a comment • edited Loading

Choose a reason for hiding this comment

ValarDragon commented Aug 27, 2018 • edited Loading

rigelrozanski commented Aug 27, 2018 • edited Loading

cwgoes commented Aug 28, 2018

cwgoes left a comment

Choose a reason for hiding this comment

rigelrozanski commented Aug 28, 2018

ValarDragon commented Aug 25, 2018 •

edited

Loading

alexanderbez Aug 26, 2018 •

edited

Loading

ValarDragon Aug 27, 2018 •

edited

Loading

cwgoes left a comment •

edited

Loading

rigelrozanski left a comment •

edited

Loading

ValarDragon commented Aug 27, 2018 •

edited

Loading

rigelrozanski commented Aug 27, 2018 •

edited

Loading