Fix dependencies v1 definition and conversion #2232
Conversation
There was a typo in the yaml tag for a dependency's version in porter.yaml. It has always been version, and we intended to just move reference and version underneath a new bundle field. It was accidentally set to versions, but none of the other code, tests or examples were updated to that value (yay!) This reverts the yaml tag back to version (singular). I'm not bumping the schema version since this actually matches what the schema has always been, it's just fixing a bug where version completely didn't work for a bit. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
When we are converting a porter.yaml to a bundle.json and populating the v1 dependencies extension metadata, we accidentally stopped populating AllowPrerelease (which was in use previously). Before we updated the dependency schema in preparation for a v2 of the dependency extension this value was set explicitly in the porter.yaml. When we removed it, we decided to use the mastermind/semver convention of "if a version constraint includes a prerelease, then we will match against releases", i.e. 1.0.0-0 would allow prereleases, but we missed updating the conversion logic to explicitly set AllowPrerelease in that situation. I've updated our testdata to work with the new logic and verified that we are now properly setting AllowPrerelease again. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
| @@ -401,10 +406,9 @@ func (c *ManifestConverter) generateBundleImages() map[string]bundle.Image { | |||
| return images | |||
| } | |||
|
|
|||
| func (c *ManifestConverter) generateDependencies() *cnab.Dependencies { | |||
|
|
|||
| func (c *ManifestConverter) generateDependencies() (*cnab.Dependencies, error) { | |||
There was a problem hiding this comment.
Maybe I'm missing something, I can't find where this function is returning an error?
There was a problem hiding this comment.
Sorry the I was chopping up #2099 into refactorings and while that larger PR needs to return an error this exact change here doesn't use it yet. Are you okay with me keeping that in place here so that I don't need to redo both PRs to shuffle when we introduce that change? I promise we will be returning errors by the time I finish splitting up that big PR. 😀
| @@ -421,12 +425,18 @@ func (c *ManifestConverter) generateDependencies() *cnab.Dependencies { | |||
| dependencyRef.Version = &cnab.DependencyVersion{ | |||
| Ranges: []string{dep.Bundle.Version}, | |||
| } | |||
|
|
|||
| // If we can detect that prereleases are used in the version, then set AllowPrereleases to true | |||
| v, err := semver.NewVersion(dep.Bundle.Version) | |||
There was a problem hiding this comment.
Should we return the error here if it's not nil?
There was a problem hiding this comment.
No, because we are just parsing the value to see if we can detect the use of a prerelease value. If they are using a semver constrain, the value isn't parsable as a singular version.
If we want to validate values from the porter.yaml, it should be in the Manifest.Validate function. In general though we aren't going to do that for deps since not all tags are semver values.
carolynvs
added
the
pep003-advanced-dependencies
What does this change
When I was implementing the dependencies v2 extension (advanced dependencies proposal), I realized that we had a regression in the v1 extension recently when we updated the porter.yaml schema to move reference and version underneath the bundle field, e.g.
I had to make two fixes to get the original test passing properly, plus update the test since there was a problem with closures that was making it pass when it shouldn't have.
Fix yaml tag for dependency version
There was a typo in the yaml tag for a dependency's version in porter.yaml. It has always been version, and we intended to just move reference and version underneath a new bundle field. It was accidentally set to versions, but none of the other code, tests or examples were
updated to that value (yay!)
This reverts the yaml tag back to version (singular). I'm not bumping the schema version since this actually matches what the schema has always been, it's just fixing a bug where version completely didn't work for a bit.
Set AllowPrerelease on deps v1 extension
When we are converting a porter.yaml to a bundle.json and populating the v1 dependencies extension metadata, we accidentally stopped populating AllowPrerelease (which was in use previously). Before we updated the dependency schema in preparation for a v2 of the dependency extension this value was set explicitly in the porter.yaml. When we removed it, we
decided to use the mastermind/semver convention of "if a version constraint includes a prerelease, then we will match against releases", i.e. 1.0.0-0 would allow prereleases, but we missed updating the conversion logic to explicitly set AllowPrerelease in that situation.
I've updated our testdata to work with the new logic and verified that we are now properly setting AllowPrerelease again.
What issue does it fix
This is a fix/follow-up to #2117
Notes for the reviewer
Put any questions or notes for the reviewer here.
Checklist
Reviewer Checklist