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

CI: Make CI not update the lock file #2152

Merged
merged 3 commits into from
Aug 28, 2018
Merged

CI: Make CI not update the lock file #2152

merged 3 commits into from
Aug 28, 2018

Conversation

ValarDragon
Copy link
Contributor

@ValarDragon ValarDragon commented Aug 25, 2018

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
Copy link

codecov bot commented Aug 25, 2018

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

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Thanks @ValarDragon ! Just left a small remark 👍

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

get_vendor_deps_ci:
Copy link
Contributor

@alexanderbez alexanderbez Aug 26, 2018

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Nor do I -- you can keep as is.

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@ValarDragon ValarDragon Aug 27, 2018

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

LGTM 👌

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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

@ValarDragon
Copy link
Contributor Author

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

Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

ValarDragon commented Aug 27, 2018

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
rigelrozanski previously approved these changes Aug 27, 2018
@rigelrozanski
Copy link
Contributor

rigelrozanski commented Aug 27, 2018

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.

@cwgoes
Copy link
Contributor

cwgoes commented Aug 28, 2018

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?

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

utACK but waiting for @rigelrozanski

@rigelrozanski
Copy link
Contributor

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

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.

5 participants