cmd/dep: fix vndrImporter's rev->constraint logic #1058
Conversation
* When a revision is specified, if it's tagged with a semver, use that to default a constraint. * When it's a revision, and we can't guess a constraint, use gps.Any() instead of the revision itself (never pin during import). * Use the standarized validation checks for importers.
Always specify each field in the manifest/lock, so that it's easier to tell that a test satisfies all validations.
| @@ -173,6 +173,22 @@ func (a *rootAnalyzer) Info() gps.ProjectAnalyzerInfo { | |||
| } | |||
| } | |||
|
|
|||
| // isVersion determines if the specified value is a version/tag in the project. | |||
| func isVersion(pi gps.ProjectIdentifier, value string, sm gps.SourceManager) (bool, gps.Version, error) { | |||
There was a problem hiding this comment.
Let's add a test for this function alone?
cmd/dep/vndr_importer.go
Outdated
| } | ||
| pc.Constraint, err = v.sm.InferConstraint(pkg.revision, pc.Ident) | ||
|
|
||
| isVersion, version, err := isVersion(pc.Ident, pkg.reference, v.sm) |
There was a problem hiding this comment.
A comment here would be helpful. A reference could be a version or revision. So, we need to first check if the reference is a version.
I got confused for the need of this check 😅 then looked up an example vendor.conf file.
There was a problem hiding this comment.
Yeah, the naming is confusing. I didn't even realize that a version could be stored in that field, which is why I renamed it to reference. I'll comment it up. 😀
|
@darkowlzz Fixed, commented and tested. 😊 |
cmd/dep/root_analyzer_test.go
Outdated
| } | ||
|
|
||
| if testcase.wantVersion != gotVersion { | ||
| t.Fatalf("Expected version for %s to be %s, got %s", value, testcase.wantVersion, gotVersion) |
There was a problem hiding this comment.
GOT and WNT style logging? 😅
There was a problem hiding this comment.
Is that a standard? I thought the test failure message was clear and I'm not sure what I should be changing it to. 😊
There was a problem hiding this comment.
I guess it's our standard 😛 Have been following since Sam picked that in one of my old PRs :)
t.Fatalf("unexpected isVersion result for %s: \n\t(GOT) %v \n\t(WNT) %v", value, gotIsVersion, testcase.wantIsVersion)
t.Fatalf("unexpected version for %s: \n\t(GOT) %v \n\t(WNT) %v", value, gotVersion, testcase.wantVersion)
There was a problem hiding this comment.
Thanks for the example! I have been doing my own thing and didn't know about the new standard. I'll make sure to use it going forward.
What does this do / why do we need it?
I missed this during my review of the original vndr PR. When I went to update the vndr tests to use the new standardized importer validations, it found some bugs when setting a constraint from a revision:
gps.Anyinstead of the revision itself (never pin during import).FYI, after this is in I'll fix govend's importer in a separate PR. It isn't using
gps.Anywhen the constraint can't be inferred.What should your reviewer look out for in this PR?
Anything else I missed... The new validations do a good job of catching mistakes as long as we are feeding in all the appropriate test cases. I did tweak the validations to always check everything because a) it's easier and b) helps finds bugs.
Do you need help or clarification on anything?
Nope!
Which issue(s) does this PR fix?
None.