-
Notifications
You must be signed in to change notification settings - Fork 53
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.
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
Implement upgrade edges support #275
Implement upgrade edges support #275
Conversation
04efb43
to
9892759
Compare
9892759
to
92db9ba
Compare
92db9ba
to
26e844e
Compare
fff05ac
to
a6205a7
Compare
Codecov Report
@@ Coverage Diff @@
## main #275 +/- ##
===========================================
- Coverage 80.69% 62.58% -18.12%
===========================================
Files 18 21 +3
Lines 803 890 +87
===========================================
- Hits 648 557 -91
- Misses 112 241 +129
- Partials 43 92 +49
Flags with carried forward coverage won't be shown. Click here to find out more.
|
fd5be1f
to
2da8d70
Compare
@@ -269,6 +265,27 @@ var _ = Describe("BundleEntity", func() { | |||
}) | |||
}) | |||
|
|||
Describe("Replaces", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
@@ -106,6 +110,9 @@ func getEntities(ctx context.Context, client client.Client) (input.EntityList, e | |||
if catalogScopedEntryName == bundle.Name { | |||
channelValue, _ := json.Marshal(property.Channel{ChannelName: ch.Name, Priority: 0}) | |||
props[property.TypeChannel] = string(channelValue) | |||
// TODO(jmprusi): Add the proper PropertyType for this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we address this todo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the following suggestion is a bigger refactor, and I'm not suggesting we do it now, but I think we should refactor things such that there are:
- variable source for bundles
- variable source for channels
Bundle entities would no longer carry upgrade edge or channel membership information. Rather, channel variables would incorporate bundle membership and upgrade edges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, something like my suggestion should happen almost immediately after this PR lands so that we set ourselves up for a more extensible future where different channel implementations can exist and we don't have to keep adding synthetic properties to bundles (which might have undesirable effects if/when we support more arbitrary contraint definitions in the future)
return r.bundleEntities | ||
} | ||
|
||
func NewInstalledPackageVariable(bundleImage string, bundleEntities []*olmentity.BundleEntity) *InstalledPackageVariable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add a unit test for this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a unit test was added here.
} | ||
} | ||
|
||
func (o *BundleDeploymentVariableSource) GetVariables(ctx context.Context, entitySource input.EntitySource) ([]deppy.Variable, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably have good test coverage of this function, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, CI failing due to codecov issues..
} | ||
|
||
// sort by channel and version | ||
// TODO: this is a bit of a hack and it assumes a well formed catalog. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please create issue/s for these todos and link them in the comments here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good ^^ Thank you for this! I know it was tough. I think we just need to address a couple of the todos, add issue/s for the others, and maybe a little bit more unit test coverage. Great work!
2da8d70
to
3be9c40
Compare
e87975d
to
d6084e3
Compare
Signed-off-by: Joaquim Moreno Prusi <[email protected]> Signed-off-by: Per Goncalves da Silva <[email protected]> Co-authored-by: Per Goncalves da Silva <[email protected]>
d6084e3
to
a9c0905
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
@@ -296,4 +313,46 @@ var _ = Describe("BundleEntity", func() { | |||
Expect(err.Error()).To(Equal("error determining bundle mediatype for entity 'operatorhub/prometheus/0.14.0': property 'olm.bundle.mediatype' ('badtype') could not be parsed: invalid character 'b' looking for beginning of value")) | |||
}) | |||
}) | |||
|
|||
// Increase test coverage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I would remove this or add a TODO if this is meant to be done in the future, non-blocking though.
return r.bundleEntities | ||
} | ||
|
||
func NewInstalledPackageVariable(bundleImage string, bundleEntities []*olmentity.BundleEntity) *InstalledPackageVariable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a unit test was added here.
@@ -106,6 +110,9 @@ func getEntities(ctx context.Context, client client.Client) (input.EntityList, e | |||
if catalogScopedEntryName == bundle.Name { | |||
channelValue, _ := json.Marshal(property.Channel{ChannelName: ch.Name, Priority: 0}) | |||
props[property.TypeChannel] = string(channelValue) | |||
// TODO(jmprusi): Add the proper PropertyType for this | |||
replacesValue, _ := json.Marshal(replacesProperty{Replaces: b.Replaces}) | |||
props["olm.replaces"] = string(replacesValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we move this to a constant? (olm.replaces)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - small non-blocking issue around a magic string. But we can tackle that on in the next PR ^^
c145c46
id := deppy.IdentifierFromString(fmt.Sprintf("installed package %s", bundleImage)) | ||
entityIDs := make([]deppy.Identifier, 0, len(bundleEntities)) | ||
for _, bundle := range bundleEntities { | ||
entityIDs = append(entityIDs, bundle.ID) | ||
} | ||
return &InstalledPackageVariable{ | ||
SimpleVariable: input.NewSimpleVariable(id, constraint.Mandatory(), constraint.Dependency(entityIDs...)), | ||
bundleEntities: bundleEntities, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmprusi what is the difference between this and RequiredPackageVariable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are semantically different concepts that get translated to the same sort of structure (variable with mandatory and dependency constraints). I guess we could separate that out as a concept if we want to simplify the code here a bit. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are semantically different concepts that get translated to the same sort of structure (variable with mandatory and dependency constraints).
@perdasilva Is it just semantic difference? Or are they/will they be processed differently? As far as I understnad - they are exactly the same today.
I guess we could separate that out as a concept if we want to simplify the code here a bit. wdyt?
For now I'm just trying to understand how it works. But I don't follow - what do you mean by separating? They are already separate structures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now it is. It could be that these two evolve on different paths. I meant separate out the pattern of a Variable with a Mandatory and Dependency constraints. I think it could be a common pattern. Then we can just typedef the required and installed package variables as that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification.
I meant separate out the pattern of a Variable with a Mandatory and Dependency constraints. I think it could be a common pattern. Then we can just typedef the required and installed package variables as that.
Sounds like a good idea.
Description
Adds upgrade edges support to operator-controller
Closes #268
Closes #267
Reviewer Checklist