Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

godep: Import additional packages as required #1640

Merged
merged 3 commits into from
Feb 13, 2018

Conversation

sigma
Copy link
Contributor

@sigma sigma commented Feb 7, 2018

What does this do / why do we need it?

In a godep manifest, entries in Packages are commonly used to specify extra
dependencies of a project, like tools (code generators for example).
Adding those same packages to dep's "required" entry achieves the same.

This hopefully reduces the number of manual tweaks to Gopkg.toml before a migration is "complete".

@sigma sigma requested a review from carolynvs as a code owner February 7, 2018 04:40
@sigma
Copy link
Contributor Author

sigma commented Feb 7, 2018

not sure what the test failure is, but it doesn't seem related?

Signal received: waiting for 1 ops to complete...
Signal received: waiting for 1 ops to complete...
--- FAIL: TestSlowVcs (0.00s)
    --- FAIL: TestSlowVcs/svn-repo (15.12s)
    	vcs_repo_test.go:211: Svn didn't return expected ErrRevisionUnavailable
FAIL

@carolynvs
Copy link
Collaborator

Yeeaaaah, sorry about that! Ignore the failed test, it's on our end.

Copy link
Collaborator

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Nice! I didn't know this about Godep. 👍

Would you please add unit test(s) to ./internal/importers/godep/importer_test.go verifying your change?

TestGodepConfig_Convert doesn't currently have a "WantRequired" in its testcase structure, so you can either add that to our test case structure and verify it, or feel free to add a new unit test to that file to check that the required package is set properly, and that it ignores local packages.

Thanks!

This mirrors what is done for ignored, as both fields are similar.
We expect to map non-local Packages in godep manifest to required entries in
dep.
In a godep manifest, entries in Packages are commonly used to specify extra
dependencies of a project, like tools (code generators for example).
Adding those same packages to dep's "required" entry achieves the same.
@sigma sigma force-pushed the pr/godep-import-packages branch from 8b61ed5 to 6130237 Compare February 12, 2018 20:24
@sigma
Copy link
Contributor Author

sigma commented Feb 12, 2018

@carolynvs thanks for pointing me to the right corner of dep testing ! Are the latest changes closer to what you expect?

Copy link
Collaborator

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for improving the godep importer! 💖

@carolynvs carolynvs merged commit 1dc2d8b into golang:master Feb 13, 2018
@carolynvs carolynvs mentioned this pull request Feb 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants