-
Notifications
You must be signed in to change notification settings - Fork 0
ROX-30064: Script to generate catalog-template.yaml #137
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
base: master
Are you sure you want to change the base?
ROX-30064: Script to generate catalog-template.yaml #137
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
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.
Scratched the surface a bit. Got enough comments for the first round of review.
Please expect more review rounds and more comments.
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.
Comments on inter-diff.
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.
A couple more thoughts for today. Will make another pass after you update the PR.
cmd/generate-catalog/generate.go
Outdated
validForChannel := channel.YStreamVersion != nil && | ||
entry.Version.Major() == channel.YStreamVersion.Major() && | ||
entry.Version.Minor() <= channel.YStreamVersion.Minor() |
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.
This must be consistent with what addReplaces
does (ref. versionsWithoutReplaces
). Right now it is not if you consider what happens when we'd introduce 5.0.0
. Do you have ideas how to make these two parts more consistent and at least make sure that when we change one we don't forget the other?
I at this point don't have a suggestion. Need to think about it.
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 wasn't able to find a nice solution, probably will come with some idea tomorrow
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.
If we add 5.0.0 to versionsWithoutReplaces
that means that we want to introduce yet another non versioned channel (like stable
and latest
). So it's hard to implement a nice solution unless we add a new configuration to bundles.yaml (e.g.:
...
pivot_channels:
- name: latest
start_version: 3.62.0
- name: stable
start_version: 4.0.0
- name: new
start_version: 5.0.0
...
I think it's fine to keep the same deprecation messages for deprecated non versioned (pivotal) channels
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 don't believe there are plans to do that: introduce another rolling channel for 5.0.0+. stable
will remain and will get new versions.
Co-authored-by: Misha Sugakov <[email protected]>
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.
Quick incomplete check.
cmd/generate-catalog/generate.go
Outdated
return fmt.Errorf("failed to read bundle list file: %v", err) | ||
} | ||
versions := getAllVersions(input.Images) | ||
versions := getAllVersions(config.Images) |
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.
Sorry, I don't understand your reply.
If it is more convenient for some functions to work with []*semver.Version
instead of []InputBundleImage
(and I agree it is), provide them with []*semver.Version
.
My concern is that the line versions := getAllVersions(config.Images)
is quite low-level in this high-level generateCatalogTemplateFile()
function so that it makes me feel uncomfortable about it.
My suggestion is to push it lower in the stack: either to declare Versions []*semver.Version
attribute as part of the Input
struct and populate that in the readInputFile()
call, or declare func (i *Input) Versions() []*semver.Version { ... }
to construct that slice dynamically.
Whichever way, the line versions := getAllVersions(config.Images)
will disappear from generateCatalogTemplateFile()
and that's what I advocate for.
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.
Made another incomplete pass. I still skip unit tests.
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.
Some things I spotted in the inter-diff
for i, channel := range channels { | ||
for _, entry := range channelEntries { | ||
if channelShouldHaveEntry(channel, entry) { | ||
channels[i].Entries = append(channels[i].Entries, entry) |
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.
There is a mutation of the original channel list.
I can't spot it. Please show it to me.
I see a mutation of .Entries
in blah.Entries
; blah
stays the same, doesn't it?
func TestValidateImageReferences(t *testing.T) { | ||
tests := []struct { |
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 can suggest a couple more cases. Just providing the references to test:
bare-image-name-without-registry@sha256:6cdcf20771f9c46640b466f804190d00eaf2e59caee6d420436e78b283d177bf
example.com/image:my-fancy-tag-v1@sha256:6cdcf20771f9c46640b466f804190d00eaf2e59caee6d420436e78b283d177bf
example.com/image@md5:9241e37fcf7f3f88c5e944bd46b0a268
example.com/image@sha256:0123456789
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 changed the library for image reference validation to make it possible to validate repository. I added those cases except for case number 2. Do you mean that it should fail? This tag is valid according to specification
catalog-template.yaml
from a newbundles.yaml
file.bundles.yaml
has a lists operator bundle images with versions, theoldest_supported_version
and a list ofbroken_versions
.oldest_supported_version
specifies what is the lowest supported version. Any version or channel beforeoldest_supported_version
will be marked as deprecated.broken_versions
a list of versions which should be skipped. For each broken version X.Y.Z the script adds "skips" for all versions > X.Y.Z and < X.Y+2.0checks
which validates thatcatalog-template.yaml
is up-to-date withbundles.yaml
cmd
folder is changedcatalog-template.yaml changes:
schema: olm.bundle
deprecation references for all version <oldest_supported_version
. So not only channels are deprecated.rhacs-3.63
channelskips
is only added for broken version + 2 minor versionlatest
channel has all 3.X.X versionsstable
channel has all 4.X.X versionsTesting this PR catalogs on OCP 4.19:
image:
quay.io/rhacs-eng/stackrox-operator-index@sha256:9345ef4e16b463205ab9dcf4446c5a34babc8c497ad9cbbeae327fb44b2f356d
commit: ca7b4bd
Now not only channels but versions are also deprecated.
Test locally: