Skip to content

Bug 1869441: Add skips information to Operator representation #1735

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 2 commits into from
Sep 10, 2020
Merged

Bug 1869441: Add skips information to Operator representation #1735

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5f7984
Add skips information to Operator representation
dinhxuanvu Aug 20, 2020
44ac7a7
Handle mutliple skips and add unit test
dinhxuanvu Sep 1, 2020
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
7 changes: 7 additions & 0 deletions pkg/controller/registry/resolver/operators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ type OperatorSurface interface {
Inline() bool
Dependencies() []*api.Dependency
Properties() []*api.Property
Skips() []string
}

type Operator struct {
Expand All @@ -230,6 +231,7 @@ type Operator struct {
sourceInfo *OperatorSourceInfo
dependencies []*api.Dependency
properties []*api.Property
skips []string
}

var _ OperatorSurface = &Operator{}
Expand Down Expand Up @@ -307,6 +309,7 @@ func NewOperatorFromBundle(bundle *api.Bundle, startingCSV string, sourceKey reg
sourceInfo: sourceInfo,
dependencies: dependencies,
properties: properties,
skips: bundle.Skips,
}, nil
}

Expand Down Expand Up @@ -372,6 +375,10 @@ func (o *Operator) Replaces() string {
return o.replaces
}

func (o *Operator) Skips() []string {
return o.skips
}

func (o *Operator) SetReplaces(replacing string) {
o.replaces = replacing
}
Expand Down
16 changes: 10 additions & 6 deletions pkg/controller/registry/resolver/resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ func (r *SatResolver) sortChannel(bundles []*Operator) ([]*Operator, error) {

bundleLookup := map[string]*Operator{}

// if a replacedBy b, then replacedBy[b] = a
// if a replaces b, then replacedBy[b] = a
replacedBy := map[*Operator]*Operator{}
replaces := map[*Operator]*Operator{}

Expand All @@ -531,12 +531,16 @@ func (r *SatResolver) sortChannel(bundles []*Operator) ([]*Operator, error) {
}

for _, b := range bundles {
if b.replaces == "" {
continue
if b.replaces != "" {
if r, ok := bundleLookup[b.replaces]; ok {
replacedBy[r] = b
replaces[b] = r
}
}
if r, ok := bundleLookup[b.replaces]; ok {
replacedBy[r] = b
replaces[b] = r
for _, skip := range b.skips {
if r, ok := bundleLookup[skip]; ok {
replacedBy[r] = b
Copy link
Contributor

Choose a reason for hiding this comment

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

Should replacedBy be isReplaced map[*Operator]bool? The value doesn't seem to be used, and there are a few scenarios here that could produce random results:

  • B skips A, and C replaces A
  • multiple bundles skip A

Copy link
Member

Choose a reason for hiding this comment

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

in other places that we have skips (i.e. the graphloader), this becomes an array.

replaces[*Operator][]*Operator

so replaces[name][0] is replaces and replaces[name][1..] is skips

Copy link
Member Author

@dinhxuanvu dinhxuanvu Sep 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We use replacedBy to just keep a record of which versions are replaced by either replaces or skips. This map is used to determine the head of the channel so the key is used, not the value. There should be a version that doesn't get replaced anything and that is the head.
The replaces[] is used to determine the order which we consider the versions that are replaced, not skipped because if they are skipped, they aren't eligible for installation. Hence I don't add skipped versions to replaces[]. I added the skipped[] to simply an insurance that will filter some stray skipped versions.

}
}
}

Expand Down