Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions alpha/declcfg/declcfg_to_model.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,18 @@ func ConvertToModel(cfg DeclarativeConfig) (model.Model, error) {
return nil, fmt.Errorf("error parsing bundle %q version %q: %v", b.Name, rawVersion, err)
}

// Parse release version from the package property.
var relver semver.Version
if props.Packages[0].Release != "" {
relver, err = semver.Parse(fmt.Sprintf("0.0.0-%s", props.Packages[0].Release))
Copy link
Member

Choose a reason for hiding this comment

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

This would parse successfully if the value of props.Packages[0].Release was 1+1. We would correctly get an error from line 146, but it would be confusing to a user, I think, because we're leaking that we expect the release to only contain "prerelease", which is kind of a out of left field unless you know that we just happen to use prerelease syntax for our release value specification.

I think it may be better to do something like this: https://github.com/joelanford/operator-controller/blob/2cf768082d2e4f129e6ec82740afab55201cbd8e/internal/operator-controller/bundle/versionrelease.go#L79-L101

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason that I wanted to use this approach is because I wanted to leverage the version.Compare and version.Validate (as well as PRVersion.Compare)... including all the consistent messaging that semver offers from version.Parse in addition to our layered-on validations.

if err != nil {
return nil, fmt.Errorf("error parsing bundle %q release version %q: %v", b.Name, props.Packages[0].Release, err)
}
if relver.Major != 0 || relver.Minor != 0 || relver.Patch != 0 || len(relver.Build) != 0 {
return nil, fmt.Errorf("bundle %q release version %q must only contain prerelease", b.Name, props.Packages[0].Release)
}
}

channelDefinedEntries[b.Package] = channelDefinedEntries[b.Package].Delete(b.Name)
found := false
for _, mch := range mpkg.Channels {
Expand All @@ -147,6 +159,7 @@ func ConvertToModel(cfg DeclarativeConfig) (model.Model, error) {
mb.Objects = b.Objects
mb.PropertiesP = props
mb.Version = ver
mb.Release = relver
}
}
if !found {
Expand Down
64 changes: 64 additions & 0 deletions alpha/declcfg/declcfg_to_model_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,70 @@ func TestConvertToModel(t *testing.T) {
},
},
},
{
name: "Error/InvalidReleaseVersion",
assertion: hasError(`error parsing bundle "foo.v0.1.0" release version "!!!": Invalid character(s) found in prerelease "!!!"`),
cfg: DeclarativeConfig{
Packages: []Package{newTestPackage("foo", "alpha", svgSmallCircle)},
Channels: []Channel{newTestChannel("foo", "alpha", ChannelEntry{Name: testBundleName("foo", "0.1.0")})},
Bundles: []Bundle{newTestBundle("foo", "0.1.0", func(b *Bundle) {
b.Properties = []property.Property{
property.MustBuildPackageRelease("foo", "0.1.0", "!!!"),
}
})},
},
},
{
name: "Error/InvalidBundleNormalizedName",
assertion: hasError(`invalid index:
└── invalid package "foo":
└── invalid channel "alpha":
└── invalid bundle "foo.v0.1.0-alpha.1.0.0":
└── name "foo.v0.1.0-alpha.1.0.0" does not match normalized name "foo-v0.1.0-alpha.1.0.0"`),
cfg: DeclarativeConfig{
Packages: []Package{newTestPackage("foo", "alpha", svgSmallCircle)},
Channels: []Channel{newTestChannel("foo", "alpha", ChannelEntry{Name: "foo.v0.1.0-alpha.1.0.0"})},
Bundles: []Bundle{newTestBundle("foo", "0.1.0", func(b *Bundle) {
b.Properties = []property.Property{
property.MustBuildPackageRelease("foo", "0.1.0", "alpha.1.0.0"),
}
b.Name = "foo.v0.1.0-alpha.1.0.0"
})},
},
},
{
name: "Success/ValidBundleReleaseVersion",
assertion: require.NoError,
cfg: DeclarativeConfig{
Packages: []Package{newTestPackage("foo", "alpha", svgSmallCircle)},
Channels: []Channel{newTestChannel("foo", "alpha", ChannelEntry{Name: "foo-v0.1.0-alpha.1.0.0"})},
Bundles: []Bundle{newTestBundle("foo", "0.1.0", func(b *Bundle) {
b.Properties = []property.Property{
property.MustBuildPackageRelease("foo", "0.1.0", "alpha.1.0.0"),
}
b.Name = "foo-v0.1.0-alpha.1.0.0"
})},
},
},
{
name: "Error/BundleReleaseWithBuildMetadata",
assertion: hasError(`invalid index:
└── invalid package "foo":
└── invalid channel "alpha":
└── invalid bundle "foo.v0.1.0+alpha.1.0.0-0.0.1":
├── name "foo.v0.1.0+alpha.1.0.0-0.0.1" does not match normalized name "foo-v0.1.0+alpha.1.0.0-0.0.1"
└── cannot use build metadata in version with a release version`),
cfg: DeclarativeConfig{
Packages: []Package{newTestPackage("foo", "alpha", svgSmallCircle)},
Channels: []Channel{newTestChannel("foo", "alpha", ChannelEntry{Name: "foo.v0.1.0+alpha.1.0.0-0.0.1"})},
Bundles: []Bundle{newTestBundle("foo", "0.1.0", func(b *Bundle) {
b.Properties = []property.Property{
property.MustBuildPackageRelease("foo", "0.1.0+alpha.1.0.0", "0.0.1"),
}
b.Name = "foo.v0.1.0+alpha.1.0.0-0.0.1"
})},
},
},
}

for _, s := range specs {
Expand Down
57 changes: 50 additions & 7 deletions alpha/model/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package model
import (
"errors"
"fmt"
"slices"
"sort"
"strings"

Expand Down Expand Up @@ -103,24 +104,24 @@ func (m *Package) Validate() error {
}

func (m *Package) validateUniqueBundleVersions() error {
versionsMap := map[string]semver.Version{}
versionsMap := map[string]string{}
bundlesWithVersion := map[string]sets.Set[string]{}
for _, ch := range m.Channels {
for _, b := range ch.Bundles {
versionsMap[b.Version.String()] = b.Version
if bundlesWithVersion[b.Version.String()] == nil {
bundlesWithVersion[b.Version.String()] = sets.New[string]()
versionsMap[b.VersionString()] = b.VersionString()
if bundlesWithVersion[b.VersionString()] == nil {
bundlesWithVersion[b.VersionString()] = sets.New[string]()
}
bundlesWithVersion[b.Version.String()].Insert(b.Name)
bundlesWithVersion[b.VersionString()].Insert(b.Name)
}
}

versionsSlice := maps.Values(versionsMap)
semver.Sort(versionsSlice)
slices.Sort(versionsSlice)

var errs []error
for _, v := range versionsSlice {
bundles := sets.List(bundlesWithVersion[v.String()])
bundles := sets.List(bundlesWithVersion[v])
if len(bundles) > 1 {
errs = append(errs, fmt.Errorf("{%s: [%s]}", v, strings.Join(bundles, ", ")))
}
Expand Down Expand Up @@ -331,6 +332,41 @@ type Bundle struct {
// These fields are used to compare bundles in a diff.
PropertiesP *property.Properties
Version semver.Version
Release semver.Version
}

func (b *Bundle) VersionString() string {
if len(b.Release.Pre) > 0 {
pres := []string{}
for _, pre := range b.Release.Pre {
pres = append(pres, pre.String())
}
relString := strings.Join(pres, ".")
return strings.Join([]string{b.Version.String(), relString}, "-")
}
return b.Version.String()
}

func (b *Bundle) normalizeName() string {
// if the bundle has release versioning, then the name must include this in standard form:
// <package-name>-v<version>-<release version>
// if no release versioning exists, then just return the bundle name
if len(b.Release.Pre) > 0 {
return strings.Join([]string{b.Package.Name, "v" + b.VersionString()}, "-")
}
return b.Name
}

// order by version, then
// release, if present
func (b *Bundle) Compare(other *Bundle) int {
if b.Name == other.Name {
return 0
}
if b.Version.NE(other.Version) {
return b.Version.Compare(other.Version)
}
return b.Release.Compare(other.Release)
}

func (b *Bundle) Validate() error {
Expand All @@ -339,6 +375,9 @@ func (b *Bundle) Validate() error {
if b.Name == "" {
result.subErrors = append(result.subErrors, errors.New("name must be set"))
}
if b.Name != b.normalizeName() {
result.subErrors = append(result.subErrors, fmt.Errorf("name %q does not match normalized name %q", b.Name, b.normalizeName()))
}
if b.Channel == nil {
result.subErrors = append(result.subErrors, errors.New("channel must be set"))
}
Expand Down Expand Up @@ -379,6 +418,10 @@ func (b *Bundle) Validate() error {
result.subErrors = append(result.subErrors, fmt.Errorf("invalid deprecation: %v", err))
}

if len(b.Version.Build) > 0 && len(b.Release.Pre) > 0 {
result.subErrors = append(result.subErrors, fmt.Errorf("cannot use build metadata in version with a release version"))
}

return result.orNil()
}

Expand Down
36 changes: 36 additions & 0 deletions alpha/model/model_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/base64"
"encoding/json"
"errors"
"fmt"
"testing"

"github.com/blang/semver/v4"
Expand Down Expand Up @@ -288,6 +289,41 @@ func TestValidators(t *testing.T) {
},
assertion: hasError(`duplicate versions found in bundles: [{0.0.1: [anakin.v0.0.1, anakin.v0.0.2]} {1.0.1: [anakin.v1.0.1, anakin.v1.0.2]}]`),
},
{
name: "Package/Error/DuplicateBundleVersionsReleases",
v: &Package{
Name: "anakin",
Channels: map[string]*Channel{
"light": {
Package: pkg,
Name: "light",
Bundles: map[string]*Bundle{
"anakin.v0.0.1": {Name: "anakin.v0.0.1", Version: semver.MustParse("0.0.1")},
"anakin.v0.0.2": {Name: "anakin.v0.0.2", Version: semver.MustParse("0.0.1")},
"anakin-v0.0.1-hotfix.0.0.1": {Name: "anakin.v0.0.1", Version: semver.MustParse("0.0.1"), Release: semver.MustParse(fmt.Sprintf("0.0.0-%s", "100")), Package: pkg},
"anakin-v0.0.2-hotfix.0.0.1": {Name: "anakin.v0.0.2", Version: semver.MustParse("0.0.1"), Release: semver.MustParse(fmt.Sprintf("0.0.0-%s", "100")), Package: pkg},
},
},
},
},
assertion: hasError(`duplicate versions found in bundles: [{0.0.1: [anakin.v0.0.1, anakin.v0.0.2]} {0.0.1-100: [anakin.v0.0.1, anakin.v0.0.2]}]`),
},
{
name: "Package/Error/BundleReleaseNormalizedName",
v: &Package{
Name: "anakin",
Channels: map[string]*Channel{
"light": {
Package: pkg,
Name: "light",
Bundles: map[string]*Bundle{
"anakin.v0.0.1.alpha1": {Name: "anakin.v0.0.1.alpha1", Version: semver.MustParse("0.0.1"), Release: semver.MustParse(fmt.Sprintf("0.0.0-%s", "alpha1")), Package: pkg},
},
},
},
},
assertion: hasError(`name "anakin.v0.0.1.alpha1" does not match normalized name "anakin-v0.0.1-alpha1"`),
},
{
name: "Package/Error/NoDefaultChannel",
v: &Package{
Expand Down
4 changes: 4 additions & 0 deletions alpha/property/property.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ func (p Property) String() string {
type Package struct {
PackageName string `json:"packageName"`
Version string `json:"version"`
Release string `json:"release,omitzero"`
}

// NOTICE: The Channel properties are for internal use only.
Expand Down Expand Up @@ -247,6 +248,9 @@ func jsonMarshal(p interface{}) ([]byte, error) {
func MustBuildPackage(name, version string) Property {
return MustBuild(&Package{PackageName: name, Version: version})
}
func MustBuildPackageRelease(name, version, relVersion string) Property {
return MustBuild(&Package{PackageName: name, Version: version, Release: relVersion})
}
func MustBuildPackageRequired(name, versionRange string) Property {
return MustBuild(&PackageRequired{name, versionRange})
}
Expand Down
28 changes: 23 additions & 5 deletions alpha/property/property_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,12 +132,12 @@ func TestParse(t *testing.T) {
},
expectProps: &Properties{
Packages: []Package{
{"package1", "0.1.0"},
{"package2", "0.2.0"},
{PackageName: "package1", Version: "0.1.0"},
{PackageName: "package2", Version: "0.2.0"},
},
PackagesRequired: []PackageRequired{
{"package3", ">=1.0.0 <2.0.0-0"},
{"package4", ">=2.0.0 <3.0.0-0"},
{PackageName: "package3", VersionRange: ">=1.0.0 <2.0.0-0"},
{PackageName: "package4", VersionRange: ">=2.0.0 <3.0.0-0"},
},
GVKs: []GVK{
{"group", "Kind1", "v1"},
Expand Down Expand Up @@ -206,10 +206,28 @@ func TestBuild(t *testing.T) {
specs := []spec{
{
name: "Success/Package",
input: &Package{"name", "0.1.0"},
input: &Package{PackageName: "name", Version: "0.1.0"},
assertion: require.NoError,
expectedProperty: propPtr(MustBuildPackage("name", "0.1.0")),
},
{
name: "Success/Package-ReleaseVersionNumber",
input: &Package{PackageName: "name", Version: "0.1.0", Release: "1"},
assertion: require.NoError,
expectedProperty: propPtr(MustBuildPackageRelease("name", "0.1.0", "1")),
},
{
name: "Success/Package-ReleaseVersionAlpha",
input: &Package{PackageName: "name", Version: "0.1.0", Release: "gamma"},
assertion: require.NoError,
expectedProperty: propPtr(MustBuildPackageRelease("name", "0.1.0", "gamma")),
},
{
name: "Success/Package-ReleaseVersionMixed",
input: &Package{PackageName: "name", Version: "0.1.0", Release: "gamma1"},
assertion: require.NoError,
expectedProperty: propPtr(MustBuildPackageRelease("name", "0.1.0", "gamma1")),
},
{
name: "Success/PackageRequired",
input: &PackageRequired{"name", ">=0.1.0"},
Expand Down
26 changes: 19 additions & 7 deletions alpha/template/converter/converter.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,34 @@ import (
"sigs.k8s.io/yaml"

"github.com/operator-framework/operator-registry/alpha/template/basic"
"github.com/operator-framework/operator-registry/alpha/template/substitutes"
"github.com/operator-framework/operator-registry/pkg/image"
)

type Converter struct {
FbcReader io.Reader
OutputFormat string
Registry image.Registry
FbcReader io.Reader
OutputFormat string
Registry image.Registry
DestinationTemplateType string // TODO: when we have a template factory, we can pass it here
}

func (c *Converter) Convert() error {
bt, err := basic.FromReader(c.FbcReader)
if err != nil {
return err
var b []byte
switch c.DestinationTemplateType {
case "basic":
bt, err := basic.FromReader(c.FbcReader)
if err != nil {
return err
}
b, _ = json.MarshalIndent(bt, "", " ")
case "substitutes":
st, err := substitutes.FromReader(c.FbcReader)
if err != nil {
return err
}
b, _ = json.MarshalIndent(st, "", " ")
}

b, _ := json.MarshalIndent(bt, "", " ")
if c.OutputFormat == "json" {
fmt.Fprintln(os.Stdout, string(b))
} else {
Expand Down
Loading