Skip to content

Commit

Permalink
feat(diff): change HeadsOnly mode to be an explicit setting
Browse files Browse the repository at this point in the history
Currently the headsOnly mode is implilitly set if newRef is not provided.
This behavior is unclear and leads to confusion. This commit will change the current
headsOnly to be explicit setting that will be converted to package setting that will
be executed accordingly.

Signed-off-by: Vu Dinh <vudinh@outlook.com>
  • Loading branch information
dinhxuanvu committed May 12, 2022
1 parent aa8db09 commit c404b49
Show file tree
Hide file tree
Showing 6 changed files with 192 additions and 14 deletions.
3 changes: 3 additions & 0 deletions alpha/action/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ type Diff struct {
IncludeConfig DiffIncludeConfig
// IncludeAdditively catalog objects specified in IncludeConfig.
IncludeAdditively bool
// HeadsOnly is the mode that selects the head of the channels only.
HeadsOnly bool

Logger *logrus.Entry
}
Expand Down Expand Up @@ -75,6 +77,7 @@ func (diff Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) {
SkipDependencies: diff.SkipDependencies,
Includer: convertIncludeConfigToIncluder(diff.IncludeConfig),
IncludeAdditively: diff.IncludeAdditively,
HeadsOnly: diff.HeadsOnly,
}
diffModel, err := g.Run(oldModel, newModel)
if err != nil {
Expand Down
15 changes: 11 additions & 4 deletions alpha/action/diff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,9 @@ func TestDiff(t *testing.T) {
{
name: "Success/HeadsOnly",
diff: Diff{
Registry: registry,
NewRefs: []string{filepath.Join("testdata", "index-declcfgs", "latest")},
Registry: registry,
NewRefs: []string{filepath.Join("testdata", "index-declcfgs", "latest")},
HeadsOnly: true,
},
expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-headsonly")),
assertion: require.NoError,
Expand All @@ -61,6 +62,7 @@ func TestDiff(t *testing.T) {
Packages: []DiffIncludePackage{{Name: "baz"}},
},
IncludeAdditively: true,
HeadsOnly: true,
},
expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-include-pkg")),
assertion: require.NoError,
Expand All @@ -79,6 +81,7 @@ func TestDiff(t *testing.T) {
},
},
IncludeAdditively: true,
HeadsOnly: true,
},
expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-include-channel")),
assertion: require.NoError,
Expand All @@ -97,6 +100,7 @@ func TestDiff(t *testing.T) {
},
},
IncludeAdditively: true,
HeadsOnly: true,
},
expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-include-channel")),
assertion: require.NoError,
Expand All @@ -115,6 +119,7 @@ func TestDiff(t *testing.T) {
},
},
IncludeAdditively: true,
HeadsOnly: true,
},
expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-include-channel")),
assertion: require.NoError,
Expand All @@ -134,15 +139,17 @@ func TestDiff(t *testing.T) {
},
},
IncludeAdditively: true,
HeadsOnly: true,
},
expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-include-channel")),
assertion: require.NoError,
},
{
name: "Fail/NewBundleImage",
diff: Diff{
Registry: registry,
NewRefs: []string{"test.registry/foo-operator/foo-bundle:v0.1.0"},
Registry: registry,
NewRefs: []string{"test.registry/foo-operator/foo-bundle:v0.1.0"},
HeadsOnly: true,
},
assertion: func(t require.TestingT, err error, _ ...interface{}) {
if !assert.Error(t, err) {
Expand Down
9 changes: 8 additions & 1 deletion alpha/declcfg/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ type DiffGenerator struct {
Includer DiffIncluder
// IncludeAdditively catalog objects specified in Includer in headsOnly mode.
IncludeAdditively bool
// HeadsOnly is the mode that selects the head of the channels only.
HeadsOnly bool

initOnce sync.Once
}
Expand All @@ -37,6 +39,8 @@ func (g *DiffGenerator) init() {
if g.Includer.Logger == nil {
g.Includer.Logger = g.Logger
}
// Inject headsOnly setting into DiffIncluder from command line setting
g.Includer.HeadsOnly = g.HeadsOnly
})
}

Expand Down Expand Up @@ -79,7 +83,7 @@ func (g *DiffGenerator) Run(oldModel, newModel model.Model) (model.Model, error)
return nil
}

headsOnlyMode := len(oldModel) == 0
headsOnlyMode := g.HeadsOnly
latestMode := !headsOnlyMode
isInclude := len(g.Includer.Packages) != 0

Expand Down Expand Up @@ -111,6 +115,9 @@ func (g *DiffGenerator) Run(oldModel, newModel model.Model) (model.Model, error)

case isInclude: // Add included objects to outputModel.

// Assume heads-only is false for include additively since we already have the channel heads
// in the output model.
g.Includer.HeadsOnly = false
// Add included packages/channels/bundles from newModel to outputModel.
if err := g.Includer.Run(newModel, outputModel); err != nil {
return nil, err
Expand Down
35 changes: 30 additions & 5 deletions alpha/declcfg/diff_include.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ type DiffIncluder struct {
// Packages to add.
Packages []DiffIncludePackage
Logger *logrus.Entry
// HeadsOnly is the mode that selects the head of the channels only.
// This setting will be overridden by any versions or bundles in the channels.
HeadsOnly bool
}

// DiffIncludePackage specifies a package, and optionally channels
Expand All @@ -35,6 +38,9 @@ type DiffIncludePackage struct {
// Package range setting is mutually exclusive with channel range/bundles/version
// settings.
Range semver.Range
// HeadsOnly is the mode that selects the head of the channels only.
// This setting will be overridden by any versions or bundles in the channels.
HeadsOnly bool
}

// DiffIncludeChannel specifies a channel, and optionally bundles and bundle versions
Expand Down Expand Up @@ -126,6 +132,7 @@ func (i DiffIncluder) Run(newModel, outputModel model.Model) error {

for _, ipkg := range i.Packages {
pkgLog := i.Logger.WithField("package", ipkg.Name)
ipkg.HeadsOnly = i.HeadsOnly
includeErrs = append(includeErrs, ipkg.includeNewInOutputModel(newModel, outputModel, pkgLog)...)
}
if len(includeErrs) != 0 {
Expand All @@ -146,10 +153,20 @@ func (ipkg DiffIncludePackage) includeNewInOutputModel(newModel, outputModel mod
}
pkgLog := logger.WithField("package", newPkg.Name)

// No channels or versions were specified, meaning "include the full package".
// No range, channels or versions were specified
if len(ipkg.Channels) == 0 && len(ipkg.AllChannels.Versions) == 0 && len(ipkg.AllChannels.Bundles) == 0 && ipkg.Range == nil {
outputModel[ipkg.Name] = newPkg
return nil
// heads-only false, meaning "include the full package".
if !ipkg.HeadsOnly {
outputModel[ipkg.Name] = newPkg
return nil
}
// heads-only true, get the head of every channel in the package
for _, c := range newPkg.Channels {
newCh := DiffIncludeChannel{
Name: c.Name,
}
ipkg.Channels = append(ipkg.Channels, newCh)
}
}

outputPkg := copyPackageNoChannels(newPkg)
Expand Down Expand Up @@ -197,16 +214,24 @@ func (ipkg DiffIncludePackage) includeNewInOutputModel(newModel, outputModel mod
chLog := pkgLog.WithField("channel", newCh.Name)

var bundles []*model.Bundle
var head *model.Bundle
var err error
if ich.Range != nil {
// No versions have been specified, but heads-only set to true, get the channel head only.
switch {
case ipkg.HeadsOnly && len(ich.Versions) == 0 && len(ich.Bundles) == 0 && ich.Range == nil:
head, err = newCh.Head()
bundles = append(bundles, head)
case ich.Range != nil:
bundles, err = getBundlesForRange(newCh, ich.Range, chLog)
} else {
default:
bundles, err = getBundlesForVersions(newCh, ich.Versions, ich.Bundles, chLog, skipMissingBundleForChannels[newCh.Name])
}

if err != nil {
ierrs = append(ierrs, fmt.Errorf("[package=%q channel=%q] %v", newPkg.Name, newCh.Name, err))
continue
}

outputCh := copyChannelNoBundles(newCh, outputPkg)
outputPkg.Channels[outputCh.Name] = outputCh
for _, b := range bundles {
Expand Down
Loading

0 comments on commit c404b49

Please sign in to comment.