Skip to content

feat(diff): change default behavior to prune by omission #819

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 5, 2021
Merged

feat(diff): change default behavior to prune by omission #819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 6 additions & 3 deletions alpha/action/diff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ type Diff struct {
SkipDependencies bool

IncludeConfig DiffIncludeConfig
// IncludeAdditively catalog objects specified in IncludeConfig.
IncludeAdditively bool

Logger *logrus.Entry
}
Expand Down Expand Up @@ -69,9 +71,10 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) {
}

g := &declcfg.DiffGenerator{
Logger: a.Logger,
SkipDependencies: a.SkipDependencies,
Includer: convertIncludeConfigToIncluder(a.IncludeConfig),
Logger: a.Logger,
SkipDependencies: a.SkipDependencies,
Includer: convertIncludeConfigToIncluder(a.IncludeConfig),
IncludeAdditively: a.IncludeAdditively,
}
diffModel, err := g.Run(oldModel, newModel)
if err != nil {
Expand Down
5 changes: 5 additions & 0 deletions alpha/action/diff_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ func TestDiff(t *testing.T) {
IncludeConfig: DiffIncludeConfig{
Packages: []DiffIncludePackage{{Name: "baz"}},
},
IncludeAdditively: true,
},
expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-include-pkg")),
assertion: require.NoError,
Expand All @@ -77,6 +78,7 @@ func TestDiff(t *testing.T) {
},
},
},
IncludeAdditively: true,
},
expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-include-channel")),
assertion: require.NoError,
Expand All @@ -94,6 +96,7 @@ func TestDiff(t *testing.T) {
},
},
},
IncludeAdditively: true,
},
expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-include-channel")),
assertion: require.NoError,
Expand All @@ -111,6 +114,7 @@ func TestDiff(t *testing.T) {
},
},
},
IncludeAdditively: true,
},
expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-include-channel")),
assertion: require.NoError,
Expand All @@ -129,6 +133,7 @@ func TestDiff(t *testing.T) {
},
},
},
IncludeAdditively: true,
Copy link
Member

@kevinrizza kevinrizza Nov 5, 2021

Choose a reason for hiding this comment

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

Do we want to add some test cases where IncludeAdditively is false for explicit output comparison?

Copy link
Member Author

@estroz estroz Nov 5, 2021

Choose a reason for hiding this comment

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

The tests I added elsewhere cover this case. These are mostly to ensure input is parsed and output is rendered correctly with real files.

},
expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-include-channel")),
assertion: require.NoError,
Expand Down
160 changes: 99 additions & 61 deletions alpha/declcfg/diff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ type DiffGenerator struct {
SkipDependencies bool
// Includer for adding catalog objects to Run() output.
Includer DiffIncluder
// IncludeAdditively catalog objects specified in Includer in headsOnly mode.
IncludeAdditively bool

initOnce sync.Once
}
Expand All @@ -32,13 +34,21 @@ func (g *DiffGenerator) init() {
if g.Logger == nil {
g.Logger = &logrus.Entry{}
}
if g.Includer.Logger == nil {
g.Includer.Logger = g.Logger
}
})
}

// Run returns a Model containing everything in newModel not in oldModel,
// and all bundles that exist in oldModel but are different in newModel.
// If oldModel is empty, only channel heads in newModel's packages are
// added to the output Model. All dependencies not in oldModel are also added.
// Run returns a Model containing a subset of catalog objects in newModel:
// - If g.Includer contains objects:
// - If g.IncludeAdditively is false, a diff will be generated only on those objects,
// depending on the mode.
// - If g.IncludeAdditionally is true, the diff will contain included objects,
// plus those added by the mode.
// - If in heads-only mode (oldModel == nil), then the heads of channels are added to the output.
// - If in latest mode, a diff between old and new Models is added to the output.
// - Dependencies are added in all modes if g.SkipDependencies is false.
func (g *DiffGenerator) Run(oldModel, newModel model.Model) (model.Model, error) {
g.init()

Expand All @@ -48,73 +58,101 @@ func (g *DiffGenerator) Run(oldModel, newModel model.Model) (model.Model, error)

outputModel := model.Model{}

// Add included packages/channels/bundles from newModel to outputModel.
// Code further down handles packages and channels included in outputModel.
g.Includer.Logger = g.Logger
if err := g.Includer.Run(newModel, outputModel); err != nil {
return nil, err
}

if len(oldModel) == 0 {
// Heads-only mode.
// Prunes old objects from outputModel if they exist.
latestPruneFromOutput := func() error {

// Make shallow copies of packages and channels that are only
// filled with channel heads.
for _, newPkg := range newModel {
// This package may have been created in the include step.
outputPkg, pkgIncluded := outputModel[newPkg.Name]
if !pkgIncluded {
outputPkg = copyPackageNoChannels(newPkg)
outputModel[outputPkg.Name] = outputPkg
}
for _, newCh := range newPkg.Channels {
if _, chIncluded := outputPkg.Channels[newCh.Name]; chIncluded {
// Head (and other bundles) were added in the include step.
continue
}
outputCh := copyChannelNoBundles(newCh, outputPkg)
outputPkg.Channels[outputCh.Name] = outputCh
head, err := newCh.Head()
if err != nil {
return nil, err
}
outputBundle := copyBundle(head, outputCh, outputPkg)
outputModel.AddBundle(*outputBundle)
}
}
} else {
// Latest mode.

// Copy newModel to create an output model by deletion,
// which is more succinct than by addition and potentially
// more memory efficient.
for _, newPkg := range newModel {
if _, pkgIncluded := outputModel[newPkg.Name]; pkgIncluded {
// The user has specified the state they want this package to have in the diff
// via an inclusion entry, so the package created above should not be changed.
continue
}
outputModel[newPkg.Name] = copyPackage(newPkg)
}

// NB(estroz): if a net-new package or channel is published,
// this currently adds the entire package. I'm fairly sure
// this behavior is ok because the next diff after a new
// package is published still has only new data.
for _, outputPkg := range outputModel {
oldPkg, oldHasPkg := oldModel[outputPkg.Name]
if !oldHasPkg {
// outputPkg was already copied to outputModel above.
continue
}
if err := diffPackages(oldPkg, outputPkg); err != nil {
return nil, err
if err := pruneOldFromNewPackage(oldPkg, outputPkg); err != nil {
return err
}
if len(outputPkg.Channels) == 0 {
// Remove empty packages.
delete(outputModel, outputPkg.Name)
}
}

return nil
}

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

switch {
Copy link
Member

@kevinrizza kevinrizza Nov 5, 2021

Choose a reason for hiding this comment

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

Is a switch statement better than a nested if statement here? Worrying about the default case running or not seems like it would be easy to break on further refactor

Copy link
Member Author

@estroz estroz Nov 5, 2021

Choose a reason for hiding this comment

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

An if statement would look more confusing, and a switch statement supports future modes more intuitively.

case !g.IncludeAdditively && isInclude: // Only diff between included objects.

// Add included packages/channels/bundles from newModel to outputModel.
if err := g.Includer.Run(newModel, outputModel); err != nil {
return nil, err
}

if latestMode {
if err := latestPruneFromOutput(); err != nil {
return nil, err
}
}

case isInclude: // Add included objects to outputModel.

// Add included packages/channels/bundles from newModel to outputModel.
if err := g.Includer.Run(newModel, outputModel); err != nil {
return nil, err
}

fallthrough
default:

if headsOnlyMode { // Net-new diff of heads only.

// Make shallow copies of packages and channels that are only
// filled with channel heads.
for _, newPkg := range newModel {
// This package may have been created in the include step.
outputPkg, pkgIncluded := outputModel[newPkg.Name]
if !pkgIncluded {
outputPkg = copyPackageNoChannels(newPkg)
outputModel[outputPkg.Name] = outputPkg
}
for _, newCh := range newPkg.Channels {
if _, chIncluded := outputPkg.Channels[newCh.Name]; chIncluded {
// Head (and other bundles) were added in the include step.
continue
}
outputCh := copyChannelNoBundles(newCh, outputPkg)
outputPkg.Channels[outputCh.Name] = outputCh
head, err := newCh.Head()
if err != nil {
return nil, err
}
outputBundle := copyBundle(head, outputCh, outputPkg)
outputModel.AddBundle(*outputBundle)
}
}

} else { // Diff between old and new Model.

// Copy newModel to create an output model by deletion,
// which is more succinct than by addition.
for _, newPkg := range newModel {
if _, pkgIncluded := outputModel[newPkg.Name]; pkgIncluded {
// The user has specified the state they want this package to have in the diff
// via an inclusion entry, so the package created above should not be changed.
continue
}
outputModel[newPkg.Name] = copyPackage(newPkg)
}

if err := latestPruneFromOutput(); err != nil {
return nil, err
}

}

}

if !g.SkipDependencies {
Expand All @@ -126,8 +164,8 @@ func (g *DiffGenerator) Run(oldModel, newModel model.Model) (model.Model, error)

// Default channel may not have been copied, so set it to the new default channel here.
for _, outputPkg := range outputModel {
outputHasDefault := false
newPkg := newModel[outputPkg.Name]
var outputHasDefault bool
outputPkg.DefaultChannel, outputHasDefault = outputPkg.Channels[newPkg.DefaultChannel.Name]
if !outputHasDefault {
// Create a name-only channel since oldModel contains the channel already.
Expand All @@ -138,9 +176,9 @@ func (g *DiffGenerator) Run(oldModel, newModel model.Model) (model.Model, error)
return outputModel, nil
}

// diffPackages removes any bundles and channels from newPkg that
// pruneOldFromNewPackage prune any bundles and channels from newPkg that
// are in oldPkg, but not those that differ in any way.
func diffPackages(oldPkg, newPkg *model.Package) error {
func pruneOldFromNewPackage(oldPkg, newPkg *model.Package) error {
for _, newCh := range newPkg.Channels {
oldCh, oldHasCh := oldPkg.Channels[newCh.Name]
if !oldHasCh {
Expand Down
Loading