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

Upgrade dep version #1918

Merged
merged 2 commits into from Mar 28, 2019
Merged

Upgrade dep version #1918

merged 2 commits into from Mar 28, 2019

Conversation

kahlouie
Copy link
Contributor

Description

Upgrade dep to 0.5.1

Reviewer Notes

It appears that dep now adds a /.vendor-new file. To keep in line with how we've ignored the vendor folder prior, I've done the same here. I'm unclear if I should also be removing the original /vendor folder from our .gitignore.

Setup

n/a

Code Review Verification Steps

  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@chrisgilmerproj
Copy link
Contributor

This is going to require a change here I think: https://github.com/trussworks/circleci-docker-primary/blob/master/Dockerfile#L51-L56

@kahlouie
Copy link
Contributor Author

Thanks @chrisgilmerproj I'll get a pr up for that and block this one on that.

@kahlouie kahlouie changed the title Upgrade dep version [blocked] Upgrade dep version Mar 27, 2019
Copy link
Contributor

@reggieriser reggieriser left a comment

Choose a reason for hiding this comment

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

I tested this out locally going back and forth between old and new dep. The pre-commit hook acted as expected. With 0.5.1, I did a make clean, then built the project, then ran server tests. Seemed fine.

For what it's worth, I never saw a .vendor-new file appear. I do recall in the past seeing a vendor.orig (I think?) every now and then. Don't know that there's any harm in putting it in .gitignore though.

There's a dep FAQ about committing the vendor folder. I would keep it ignored like we do now.

I'm going to go ahead and approve this one so you're ready with it when the blocking Dockerfile PR is approved/merged.

@kahlouie kahlouie changed the title [blocked] Upgrade dep version Upgrade dep version Mar 27, 2019
@kahlouie kahlouie force-pushed the kl-upgrade-dep-version-check branch from c3a499b to 8f7ce09 Compare March 27, 2019 21:18
@codecov
Copy link

codecov bot commented Mar 27, 2019

Codecov Report

Merging #1918 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1918   +/-   ##
=======================================
  Coverage   50.81%   50.81%           
=======================================
  Files         446      446           
  Lines       19411    19411           
  Branches     1691     1691           
=======================================
  Hits         9863     9863           
  Misses       8683     8683           
  Partials      865      865

Copy link
Contributor

@chrisgilmerproj chrisgilmerproj left a comment

Choose a reason for hiding this comment

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

🚀

@kahlouie kahlouie merged commit 3ecc336 into master Mar 28, 2019
@kahlouie kahlouie deleted the kl-upgrade-dep-version-check branch March 28, 2019 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants