Use the apparentlymart/go-versions library to parse module constraints #32377
Conversation
liamcervante
force-pushed
the
liamcervante/32356
branch
from
c7a62d4
to
1f12d16
Compare
There was a problem hiding this comment.
I left some notes about details inline, but overall this seems reasonable to me.
Did you find that there wasn't a good place to add tests for this? I must admit I don't recall what the module installer's testing approach is since it's some quite old code at this point, but I hope there's at least something somewhere testing that it's possible to install a prerelease version using an exact match without any operator and so would've hoped we could add a similar test alongside that which uses the = operator to get the same effect.
internal/initwd/module_install.go
Outdated
| // We validated the version string previously, so we are quite sure | ||
| // that MustParseVersion will not crash/panic. | ||
| if !acceptableVersions.Has(versions.MustParseVersion(v.String())) { |
There was a problem hiding this comment.
I'm not sure if it's 100% safe to assume that successful parsing with hashicorp/go-version guarantees successful parsing with apparentlymart/versions. For example: I believe go-version allows a string like 1.0.0.0.0.0.0.0 (any number of numeric segments) if using its main version parser, although not if using its NewSemver function.
If we have been allowing non-semver-syntax version numbers then I suppose this could represent a compatibility regression, so I hope we haven't but I cannot guarantee it off the top of my head. Would probably be worth some testing with some of the examples from the New tests and the NewSemver tests to get a sense of what is and isn't currently accepted by our module installer, and whether all of those inputs are valid per apparentlymart/go-versions.
(If go-versions is rejecting any that seems like valid semver syntax then I would consider that to be a bug, but go-version was originally more general than semver and that would be out of scope for apparentlymart/go-versions, which is focused on the semver sense of version numbers in particular.)
There was a problem hiding this comment.
I've added a global comment discussing testing more generally. In this case specifically, it checks if the version is parseable now and writes a log message if not.
| // that MustParseVersion will not crash/panic. | ||
| if !acceptableVersions.Has(versions.MustParseVersion(v.String())) { | ||
| log.Printf("[TRACE] ModuleInstaller: %s ignoring %s because it is a pre-release and was not requested exactly", key, v) | ||
| continue |
There was a problem hiding this comment.
It's interesting to note that this effectively means that any prerelease versions announced by the registry will be subject to apparentlymart/go-versions matching before we get to the req.VersionConstraints.Check(v) call below.
I don't think this is actually a problem -- this is still more general than the direct string comparison it is replacing -- but might be worth a comment noting this in the hope of minimizing confusion for a future maintainer here?
There was a problem hiding this comment.
I've added additional comments, expanding on the context. Hopefully it's more clear now 😅
internal/initwd/module_install.go
Outdated
| // This shouldn't really happen, as we validated the constraints | ||
| // string earlier. Let's return a nice error message anyway. |
There was a problem hiding this comment.
While I agree that this should be okay, it is possible in principle that versions.MeetingConstraintsString is stricter in some cases than the hashicorp/go-versions parser.
Given that for now we're just trying to use apparentlymart/go-versions only as an extra filter for prereleases and not fully embrace it, I wonder if we should instead make this a log.Printf with [WARN] so that a regular user will never see any messaging in this case.
I think it should be safe to assume that any version constraint that represents just an exact version selection should always pass versions.MeetingConstraintsString, and so it seems pragmatic to assume that if we get any errors here then it's not just an exact match and therefore shouldn't select a prerelease.
(Note that v is under the control of the module registry rather than the user, so a user with a version constraint that is somehow valid for hashicorp/go-version but not for apparentlymart/go-versions would see this warning just because the module registry announced at least one prerelease version, regardless of whether they actually intended to use that prerelease version.)
There was a problem hiding this comment.
Done! Changed the tfdiags for a warning log message.
There was a problem hiding this comment.
So all the tests in this package are essentially acceptance tests, in that they download real modules from the registry.
I was wondering if maybe I could add prerelease tag to terraform-aws-module-installer-acc-test. Then I could add some acceptance tests around that and make use of the existing testing infrastructure.
I don't think I can add simple unit tests for this easily though. It's all embedded inside the function that is pulling from the actual registry. I'd have to go through and add mocks for the registry etc. Does this functionality exist elsewhere?
Thoughts?
| // that MustParseVersion will not crash/panic. | ||
| if !acceptableVersions.Has(versions.MustParseVersion(v.String())) { | ||
| log.Printf("[TRACE] ModuleInstaller: %s ignoring %s because it is a pre-release and was not requested exactly", key, v) | ||
| continue |
There was a problem hiding this comment.
I've added additional comments, expanding on the context. Hopefully it's more clear now 😅
internal/initwd/module_install.go
Outdated
| // We validated the version string previously, so we are quite sure | ||
| // that MustParseVersion will not crash/panic. | ||
| if !acceptableVersions.Has(versions.MustParseVersion(v.String())) { |
There was a problem hiding this comment.
I've added a global comment discussing testing more generally. In this case specifically, it checks if the version is parseable now and writes a log message if not.
internal/initwd/module_install.go
Outdated
| // This shouldn't really happen, as we validated the constraints | ||
| // string earlier. Let's return a nice error message anyway. |
There was a problem hiding this comment.
Done! Changed the tfdiags for a warning log message.
There was a problem hiding this comment.
Great!
That other repository you mentioned seems to exist purely to support the testing if three module installer here, so it seems reasonable to me to add a prerelease tag to it. I suppose the trick will be in whether the public registry webhooks for that repository are still working after all this time so that the registry will see the new tag being created and make it visible through the registry API, but just trying it seems like the easiest way to find out!
Making the prerelease tag match the current HEAD seems plausible since that should be enough to make something valid to install.
liamcervante
added
the
1.3-backport
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Update the module constraint checking logic to use the apparentlymart/go-versions library instead of the hashicorp/go-versions library. This copies the logic of the provider installer which relies entirely on the apparentlymart implementation.
A further PR could look into completely stripping out the old version with the new version for all version processing (not just constraint checking). This is a lot of work though, so this PR could be viewed as a first step in that direction, or maybe we should decide that we should fix this only with a bigger migration and I can close this PR?
Fixes #32356
Target Release
1.3.6
Draft CHANGELOG entry
BUG FIXES