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

Dep Support #993

Merged
merged 2 commits into from
Aug 30, 2017
Merged

Dep Support #993

merged 2 commits into from
Aug 30, 2017

Conversation

akutz
Copy link
Member

@akutz akutz commented Aug 29, 2017

This patch introduces support for the official Go dependency management tool, Dep. Glide support has been removed in favor of the new, official tool.

@akutz akutz added the build label Aug 29, 2017
@akutz akutz added this to the 2017.09-1 milestone Aug 29, 2017
@akutz akutz self-assigned this Aug 29, 2017
@akutz akutz requested a review from codenrhoden August 29, 2017 22:40
@codecov-io
Copy link

codecov-io commented Aug 29, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #993   +/-   ##
=======================================
  Coverage   34.11%   34.11%           
=======================================
  Files          36       36           
  Lines        2896     2896           
=======================================
  Hits          988      988           
  Misses       1806     1806           
  Partials      102      102

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 1a556e8...07ea8b0. Read the comment docs.

Copy link
Member

@codenrhoden codenrhoden left a comment

Choose a reason for hiding this comment

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

I should clarify that dep is billed as the "official experiment". Once it is the official tool, it is more likely to exist as go dep, but they weren't even committal on that.

The pedant in me would want to see this split into two commits. One that replaces Glide with the same dependency versions (or as close to it as we can get) so we can see what changes in vendor/. Ideally switching it out would have minimal changes to vendor/. Then, a second commit that upgrades dependencies where we've chosen to do so.

I'm not sure how feasible that is, and I'm certainly not requesting it. I think the hardest part in achieving that would be that we are going to back to having Gopkg.toml only list immediate dependencies, not transitive ones.

@akutz
Copy link
Member Author

akutz commented Aug 30, 2017

Hi @codenrhoden,

The pedant in me would want to see this split into two commits. One that replaces Glide with the same dependency versions (or as close to it as we can get) so we can see what changes in vendor/. Ideally switching it out would have minimal changes to vendor/. Then, a second commit that upgrades dependencies where we've chosen to do so.

So I did attempt to do this. The PR's commit should not introduce any new or updated versions of dependencies. The changes you see are due to the fact that I did not attempt to recreate a Gopkg.toml that is equivalent to the old glide.yaml file with explicit, transitive dependencies. You even mention this:

I think the hardest part in achieving that would be that we are going to back to having Gopkg.toml only list immediate dependencies, not transitive ones.

I'm going to spend 30 or so minutes to see if I am able to create a Gopkg.toml that is equivalent to the deprecated glide.yaml, transitive dependencies and all. At least then we should see an initial diff with the only changes being the manifest files and not vendor/. Mostly anyway. There is also an issue with the go-yaml dependency.

@akutz akutz force-pushed the feature/dep branch 2 times, most recently from 9730d18 to 0a06d73 Compare August 30, 2017 01:39
This patch introduces support for the official Go dependency management
tool, Dep (https://github.com/golang/dep). Glide support has been
removed in favor of the new, official tool.
@akutz
Copy link
Member Author

akutz commented Aug 30, 2017

Hi @codenrhoden,

I'm going to add a second commit to this PR that updates the versions of the dependencies by virtue of undoing the overrides in Gopkg.toml. The reason I'm adding the commit to this same PR is so we can easily revert the PR in order to back out all these changes if Dep + stripping nested vendor breaks anything.

@codenrhoden
Copy link
Member

Awesome. I really like being able to see what versions changed between the two commits. And having the transitive deps out of the .toml file is a big win.

Scrolling through all the deps, I think the only thing I noticed is that we don't have Cobra as a direct dependency, so it gets pointed to master, which is a little surprising. Maybe something to think about locking down in the future.

This patch hands over the job of tracking transitive dependencies to the
Dep tool. This patch also updates dependencies to their latest versions
with the exception of the Azure SDK and Autorest packages. Attempts to
update those resulted in build errors.
@akutz
Copy link
Member Author

akutz commented Aug 30, 2017

Hi @codenrhoden,

I force pushed this PR with the second commit updated to reflect Cobra and PFlag as direct entries in the Gopkg.toml file per your suggestion. The reason for their absence in the first place is @spf13 and his project maintainers' decision to forego tagged releases. Without tagged releases the Cobra and PFlag dependencies simply point at master unless we lock them to a specific revision.

@akutz akutz merged commit 3443713 into rexray:master Aug 30, 2017
@akutz akutz deleted the feature/dep branch August 30, 2017 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants