Skip to content

Commit cb43dd4

Browse files
grokspawngrokspawn
committed
review updates
Signed-off-by: grokspawn <[email protected]>
1 parent d28cca8 commit cb43dd4

File tree

6 files changed

+59
-26
lines changed

6 files changed

+59
-26
lines changed

‎alpha/model/model.go‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -53,60 +53,60 @@ type Package struct {
5353
Deprecation *Deprecation
5454
}
5555

56-
func (m *Package) Validate() error {
57-
result := newValidationError(fmt.Sprintf("invalid package %q", m.Name))
56+
func (p *Package) Validate() error {
57+
result := newValidationError(fmt.Sprintf("invalid package %q", p.Name))
5858

59-
if m.Name == "" {
59+
if p.Name == "" {
6060
result.subErrors = append(result.subErrors, errors.New("package name must not be empty"))
6161
}
6262

63-
if err := m.Icon.Validate(); err != nil {
63+
if err := p.Icon.Validate(); err != nil {
6464
result.subErrors = append(result.subErrors, err)
6565
}
6666

67-
if m.DefaultChannel == nil {
67+
if p.DefaultChannel == nil {
6868
result.subErrors = append(result.subErrors, fmt.Errorf("default channel must be set"))
6969
}
7070

71-
if len(m.Channels) == 0 {
71+
if len(p.Channels) == 0 {
7272
result.subErrors = append(result.subErrors, fmt.Errorf("package must contain at least one channel"))
7373
}
7474

7575
foundDefault := false
76-
for name, ch := range m.Channels {
76+
for name, ch := range p.Channels {
7777
if name != ch.Name {
7878
result.subErrors = append(result.subErrors, fmt.Errorf("channel key %q does not match channel name %q", name, ch.Name))
7979
}
8080
if err := ch.Validate(); err != nil {
8181
result.subErrors = append(result.subErrors, err)
8282
}
83-
if ch == m.DefaultChannel {
83+
if ch == p.DefaultChannel {
8484
foundDefault = true
8585
}
86-
if ch.Package != m {
86+
if ch.Package != p {
8787
result.subErrors = append(result.subErrors, fmt.Errorf("channel %q not correctly linked to parent package", ch.Name))
8888
}
8989
}
9090

91-
if err := m.validateUniqueBundleVersions(); err != nil {
91+
if err := p.validateUniqueBundleVersions(); err != nil {
9292
result.subErrors = append(result.subErrors, err)
9393
}
9494

95-
if m.DefaultChannel != nil && !foundDefault {
96-
result.subErrors = append(result.subErrors, fmt.Errorf("default channel %q not found in channels list", m.DefaultChannel.Name))
95+
if p.DefaultChannel != nil && !foundDefault {
96+
result.subErrors = append(result.subErrors, fmt.Errorf("default channel %q not found in channels list", p.DefaultChannel.Name))
9797
}
9898

99-
if err := m.Deprecation.Validate(); err != nil {
99+
if err := p.Deprecation.Validate(); err != nil {
100100
result.subErrors = append(result.subErrors, fmt.Errorf("invalid deprecation: %v", err))
101101
}
102102

103103
return result.orNil()
104104
}
105105

106-
func (m *Package) validateUniqueBundleVersions() error {
106+
func (p *Package) validateUniqueBundleVersions() error {
107107
versionsMap := map[string]string{}
108108
bundlesWithVersion := map[string]sets.Set[string]{}
109-
for _, ch := range m.Channels {
109+
for _, ch := range p.Channels {
110110
for _, b := range ch.Bundles {
111111
versionsMap[b.VersionString()] = b.VersionString()
112112
if bundlesWithVersion[b.VersionString()] == nil {

‎alpha/model/model_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -298,10 +298,10 @@ func TestValidators(t *testing.T) {
298298
Package: pkg,
299299
Name: "light",
300300
Bundles: map[string]*Bundle{
301-
"anakin.v0.0.1": {Name: "anakin.v0.0.1", Version: semver.MustParse("0.0.1")},
302-
"anakin.v0.0.2": {Name: "anakin.v0.0.2", Version: semver.MustParse("0.0.1")},
303-
"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},
304-
"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},
301+
"anakin.v0.0.1": {Name: "anakin.v0.0.1", Version: semver.MustParse("0.0.1")},
302+
"anakin.v0.0.2": {Name: "anakin.v0.0.2", Version: semver.MustParse("0.0.1")},
303+
"anakin-v0.0.1-100": {Name: "anakin.v0.0.1", Version: semver.MustParse("0.0.1"), Release: semver.MustParse(fmt.Sprintf("0.0.0-%s", "100")), Package: pkg},
304+
"anakin-v0.0.2-100": {Name: "anakin.v0.0.2", Version: semver.MustParse("0.0.1"), Release: semver.MustParse(fmt.Sprintf("0.0.0-%s", "100")), Package: pkg},
305305
},
306306
},
307307
},

‎alpha/template/converter/converter.go‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,19 +22,31 @@ type Converter struct {
2222

2323
func (c *Converter) Convert() error {
2424
var b []byte
25+
var err error
2526
switch c.DestinationTemplateType {
2627
case "basic":
27-
bt, err := basic.FromReader(c.FbcReader)
28+
var bt *basic.BasicTemplate
29+
bt, err = basic.FromReader(c.FbcReader)
30+
if err != nil {
31+
return err
32+
}
33+
b, err = json.MarshalIndent(bt, "", " ")
2834
if err != nil {
2935
return err
3036
}
31-
b, _ = json.MarshalIndent(bt, "", " ")
3237
case "substitutes":
33-
st, err := substitutes.FromReader(c.FbcReader)
38+
var st *substitutes.SubstitutesForTemplate
39+
st, err = substitutes.FromReader(c.FbcReader)
40+
if err != nil {
41+
return err
42+
}
43+
b, err = json.MarshalIndent(st, "", " ")
3444
if err != nil {
3545
return err
3646
}
37-
b, _ = json.MarshalIndent(st, "", " ")
47+
default:
48+
// usage pattern prevents us from getting here, so if we do it's a programmer failure and we should panic
49+
panic(fmt.Sprintf("unknown template type %q", c.DestinationTemplateType))
3850
}
3951

4052
if c.OutputFormat == "json" {

‎alpha/template/substitutes/substitutes.go‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,22 @@ import (
1212
"github.com/operator-framework/operator-registry/alpha/declcfg"
1313
)
1414

15+
// Template implements a catalog template to make the substitutesFor mechanics less error prone.
16+
// It provides a customizable RenderBundle function that is used to produce declarative config for a supplied bundle.
1517
type Template struct {
1618
RenderBundle func(context.Context, string) (*declcfg.DeclarativeConfig, error)
1719
}
1820

21+
// Substitute defines a replacement relationship between an existing bundle name and a superceding bundle image pullspec.
22+
// Since registry+v0 graphs are bundle name based, this uses the name instead of a version.
1923
type Substitute struct {
2024
Name string `json:"name"` // the bundle image pullspec to substitute
2125
Base string `json:"base"` // the bundle name to substitute for
2226
}
2327

28+
// SubstitutesForTemplate represents a template for bundle substitutions.
29+
// It contains the schema identifier, an input declarative config, and substitution mappings
30+
// that define how bundles should be replaced in upgrade graphs.
2431
type SubstitutesForTemplate struct {
2532
Schema string `json:"schema"`
2633
Entries []*declcfg.Meta `json:"entries"`
@@ -95,6 +102,12 @@ func (t Template) processSubstitution(ctx context.Context, cfg *declcfg.Declarat
95102
return fmt.Errorf("failed to render bundle image reference %q: %v", substitution.Name, err)
96103
}
97104

105+
// normally, we'd rely on RenderBundle to represent any failure via err, but since this is comes from input,
106+
// we need to perform more validation of the results here before processing them
107+
if substituteCfg == nil || len(substituteCfg.Bundles) == 0 {
108+
return fmt.Errorf("rendered bundle image reference %q contains no bundles", substitution.Name)
109+
}
110+
98111
substituteBundle := &substituteCfg.Bundles[0]
99112

100113
// Iterate over all channels
@@ -167,7 +180,7 @@ func (t Template) processSubstitution(ctx context.Context, cfg *declcfg.Declarat
167180
return nil
168181
}
169182

170-
// FromReader reads FBC from a reader and generates a BasicTemplate from it
183+
// FromReader reads FBC from a reader and generates a SubstitutesForTemplate from it
171184
func FromReader(r io.Reader) (*SubstitutesForTemplate, error) {
172185
var entries []*declcfg.Meta
173186
if err := declcfg.WalkMetasReader(r, func(meta *declcfg.Meta, err error) error {

‎pkg/lib/validation/bundle.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,18 @@ func validateBundle(bundle *registry.Bundle) errors.ManifestResult {
3939
result.Add(errors.ErrInvalidParse("error getting bundle CSV version", err))
4040
return result
4141
}
42-
if _, err = csv.GetRelease(); err != nil {
42+
release, err := csv.GetRelease()
43+
if err != nil {
4344
result.Add(errors.ErrInvalidParse("error getting bundle CSV release version", err))
4445
return result
4546
}
47+
substitutesFor := csv.GetSubstitutesFor()
48+
if release != "" && substitutesFor != "" {
49+
result.Add(errors.ErrInvalidBundle(
50+
fmt.Sprintf("bundle %q cannot have both a release version (%q) and olm.substitutesFor annotation (%q)", bundle.Name, release, substitutesFor),
51+
registry.DefinitionKey{},
52+
))
53+
}
4654
return result
4755
}
4856

‎pkg/registry/parse.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,7 @@ func extractReleaseVersionFromBuildMetadata(substitutesFor string) (string, stri
280280
version = parts[0]
281281
release = parts[1]
282282
} else {
283-
return "", "", fmt.Errorf("no release version expressed as build metadata: %q", version)
283+
return "", "", fmt.Errorf("no release version expressed as build metadata: %q", substitutesFor)
284284
}
285285
return version, release, nil
286286
}

0 commit comments

Comments
 (0)