Skip to content

Conversation

kurlov
Copy link
Member

@kurlov kurlov commented Jul 1, 2025

  • Adds a new Go script to generates catalog-template.yaml from a new bundles.yaml file.
  • The bundles.yaml has a lists operator bundle images with versions, the oldest_supported_version and a list of broken_versions.
    • oldest_supported_version specifies what is the lowest supported version. Any version or channel before oldest_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.0
  • Add CI step to checks which validates that catalog-template.yaml is up-to-date with bundles.yaml
  • Add CI to run go unit tests if cmd folder is changed

catalog-template.yaml changes:

  • Now it's autogenerated
  • All yaml anchors are dropped
  • Add schema: olm.bundle deprecation references for all version < oldest_supported_version. So not only channels are deprecated.
  • Add rhacs-3.63 channel
  • skips is only added for broken version + 2 minor version
  • Channels keeps all previous version within the same major version (e.g. 3.66 channel has all version <= 3.66.x)
  • latest channel has all 3.X.X versions
  • stable channel has all 4.X.X versions

Testing 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.

deprecations

Test locally:

make generate-catalog-template
make clean && make valid-catalogs

This comment was marked as off-topic.

@kurlov kurlov changed the title WIP script to generate catalog-template.yaml DO NOT REVIEW YET: script to generate catalog-template.yaml Jul 1, 2025
@kurlov kurlov changed the title DO NOT REVIEW YET: script to generate catalog-template.yaml WIP: script to generate catalog-template.yaml Jul 1, 2025
@kurlov kurlov changed the title WIP: script to generate catalog-template.yaml Script to generate catalog-template.yaml Jul 9, 2025
@kurlov kurlov marked this pull request as ready for review July 10, 2025 12:23
@kurlov kurlov changed the title Script to generate catalog-template.yaml ROX-30064: Script to generate catalog-template.yaml Jul 10, 2025
@kurlov kurlov requested review from mclasmeier and porridge July 11, 2025 12:57
@msugakov msugakov self-requested a review July 11, 2025 13:46
Copy link
Contributor

@msugakov msugakov left a 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.

@kurlov kurlov requested a review from msugakov July 15, 2025 10:05
@kurlov kurlov requested a review from msugakov August 18, 2025 14:32
Copy link
Contributor

@msugakov msugakov left a 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.

Copy link
Contributor

@msugakov msugakov left a 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.

Comment on lines 232 to 234
validForChannel := channel.YStreamVersion != nil &&
entry.Version.Major() == channel.YStreamVersion.Major() &&
entry.Version.Minor() <= channel.YStreamVersion.Minor()
Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Member Author

@kurlov kurlov Aug 20, 2025

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

Copy link
Contributor

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.

@kurlov kurlov requested a review from msugakov August 19, 2025 17:28
Copy link
Contributor

@msugakov msugakov left a comment

Choose a reason for hiding this comment

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

Quick incomplete check.

return fmt.Errorf("failed to read bundle list file: %v", err)
}
versions := getAllVersions(input.Images)
versions := getAllVersions(config.Images)
Copy link
Contributor

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.

Copy link
Contributor

@msugakov msugakov left a 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.

@kurlov kurlov requested a review from msugakov August 21, 2025 15:28
Copy link
Contributor

@msugakov msugakov left a 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

Comment on lines +211 to +214
for i, channel := range channels {
for _, entry := range channelEntries {
if channelShouldHaveEntry(channel, entry) {
channels[i].Entries = append(channels[i].Entries, entry)
Copy link
Contributor

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?

Comment on lines 207 to 208
func TestValidateImageReferences(t *testing.T) {
tests := []struct {
Copy link
Contributor

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:

  1. bare-image-name-without-registry@sha256:6cdcf20771f9c46640b466f804190d00eaf2e59caee6d420436e78b283d177bf
  2. example.com/image:my-fancy-tag-v1@sha256:6cdcf20771f9c46640b466f804190d00eaf2e59caee6d420436e78b283d177bf
  3. example.com/image@md5:9241e37fcf7f3f88c5e944bd46b0a268
  4. example.com/image@sha256:0123456789

Copy link
Member Author

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

@kurlov kurlov requested a review from msugakov September 17, 2025 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants