Skip to content
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

Introspect image type and skip for bundle images #1533

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

komish
Copy link
Contributor

@komish komish commented Oct 24, 2024

Related to KFLUXBUGS-1638- This pull request modifies the ecosystem check such that it should no longer fail (or need to be worked around) if the built image is an operator bundle.

The original scope for this issue was to detect the image was a bundle and then skip the remaining checks. I went ahead and added a bundle validation call just because it seemed within reach (see KONFLUX-178). If this implementation is not desirable, I'm happy to return to the original scope (i.e. detect the image is a bundle, then skip the ecosystem application image checks).

Before:

  • User builds a bundle image
  • This task throws a failure (shows up as a ⚠️) because Preflight (check container) can't find what it's expecting.

Now:

  • The image's labels are used to determine if it's a bundle or application image (i.e. would call check operator or check container, respectively) unless explicitly set as a Task parameter.
  • Operator bundles are unpacked and have operator-sdk bundle validate called on them. The operatoframework optional suite is called with k8s version set to 1.22. This is modifiable via Task parameters, but seemed like a reasonable default for now to match pre-existing pipelines doing similar things.
  • NEW: operator bundles will produce a skipped result
  • Application images will go through the same process as before (e.g. preflight check container)

Notable:

  • This new version requires the same input parameters as the v0.1 Task (at a minimum) to reduce friction on custom pipeline definitions. All new parameters are optional.
  • The k8s-version optional parameter is set for users as a sane default. I didn't want to parse this value in shell scripts.
  • The bundle validation call uses the Preflight image because operator-sdk is available there. This implementation is a stop gap until preflight check operator can be executed against the input image (I.e. when a disposable cluster and catalog becomes available).

[EDIT: 2025-02-04] This work was rescoped to just skip for operator bundles, and validating operator bundles will be done in another way through a separate JIRA.

@openshift-ci openshift-ci bot requested review from mkosiarc and rcerven October 24, 2024 20:10
acornett21
acornett21 previously approved these changes Oct 24, 2024
@arewm
Copy link
Member

arewm commented Oct 25, 2024

A MIGRATION.md file is needed to instruct users how to upgrade to the new task version. Examples of this file can be found in other tasks which have new task versions.

@arewm
Copy link
Member

arewm commented Oct 25, 2024

/ok-to-test

@komish komish force-pushed the ecosystem-bundle-validation branch from 22bf9e3 to 88824b1 Compare October 25, 2024 17:51
@komish
Copy link
Contributor Author

komish commented Oct 25, 2024

@arewm Done - All of the new inputs are optional, so there are no required parameters to note in the migration doc. MIGRATION.md has been added.

I think I got all ShellCheck and yamllint complaints as well.

@arewm
Copy link
Member

arewm commented Oct 25, 2024

Oh! We do not currently have the ability to specify a non-breaking version bump. Since this is not a breaking change, should we just keep the task version consistent?

@komish
Copy link
Contributor Author

komish commented Oct 25, 2024

I think I'd prefer to leave it as an increment given the underlying logic has changed. I would assume that users can also rollback to 0.1 if something in the 0.2 implementation isn't working for them, which seems useful to me. Open either way.

@komish
Copy link
Contributor Author

komish commented Oct 25, 2024

Also, there are new results here - but no removed results. Not sure if that helps justify a 0.2.

@komish komish force-pushed the ecosystem-bundle-validation branch from 88824b1 to 61cb86f Compare October 30, 2024 16:41
@komish
Copy link
Contributor Author

komish commented Oct 30, 2024

61cb86f rebases off of main.

@arewm
Copy link
Member

arewm commented Oct 31, 2024

@dirgim @hongweiliu17 , you reviewed the initial preflight task. Would you be able to help review this update as well?

@arewm arewm requested a review from dirgim October 31, 2024 05:09
@komish komish force-pushed the ecosystem-bundle-validation branch from 61cb86f to e38256a Compare October 31, 2024 13:53
@komish
Copy link
Contributor Author

komish commented Oct 31, 2024

e38256a addresses lint warnings identified here.

@komish komish force-pushed the ecosystem-bundle-validation branch from e38256a to 29ad54d Compare October 31, 2024 13:54
@komish
Copy link
Contributor Author

komish commented Oct 31, 2024

29ad54d rebases off of main again.

@dhaiducek
Copy link
Contributor

What's the status here?

@nickboldt
Copy link

When can we get this merged so that my scratch-based operator-bundle image won't be tested as if it's a UBI9-based container? https://issues.redhat.com/browse/RHIDP-4220?focusedId=26086287&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-26086287

@dhaiducek
Copy link
Contributor

dhaiducek commented Nov 15, 2024

Slack support workflow initiated for this PR in #konflux-users:

@acornett21
Copy link

/lgtm

@rcerven
Copy link
Contributor

rcerven commented Nov 15, 2024

/ok-to-test

@komish komish force-pushed the ecosystem-bundle-validation branch from 910e051 to ef17f7d Compare November 15, 2024 18:29
@rcerven
Copy link
Contributor

rcerven commented Nov 15, 2024

/ok-to-test

dirgim
dirgim previously approved these changes Nov 20, 2024
Copy link
Contributor

@dirgim dirgim left a comment

Choose a reason for hiding this comment

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

LGTM

hongweiliu17
hongweiliu17 previously approved these changes Nov 21, 2024
Copy link
Contributor

@hongweiliu17 hongweiliu17 left a comment

Choose a reason for hiding this comment

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

lgtm

@dhaiducek
Copy link
Contributor

This needs another /ok-to-test because it's not authored by an owner.

@dirgim
Copy link
Contributor

dirgim commented Nov 21, 2024

/ok-to-test

@dhaiducek
Copy link
Contributor

I'm surprised this PR is still open two weeks after approval. What's blocking it from being merged?

@dirgim
Copy link
Contributor

dirgim commented Dec 3, 2024

Apologies @komish, there seemed to have been an overlooked issue with this PR - the new task version isn't referenced in the default template-build.yaml.
Note also that the READMEs for individual pipelines that use this task will need to be updated - see PR #1645 for an example.

@dhaiducek
Copy link
Contributor

@dirgim @komish Is this PR ready to go now?

@komish
Copy link
Contributor Author

komish commented Jan 27, 2025

@dhaiducek Not yet. I'll try to get this updated this week. Thanks for the reminder ping.

@komish komish dismissed stale reviews from hongweiliu17 and dirgim via 1082f75 February 4, 2025 21:22
@komish komish force-pushed the ecosystem-bundle-validation branch from 6c4bb65 to 1082f75 Compare February 4, 2025 21:22
@komish komish changed the title introspect image type and add bundle validation Introspect image type and skip for bundle images Feb 4, 2025
@komish komish force-pushed the ecosystem-bundle-validation branch 3 times, most recently from 0298a04 to 1e615d7 Compare February 4, 2025 21:31
@komish
Copy link
Contributor Author

komish commented Feb 4, 2025

/retest

@nickboldt
Copy link

Created Oct 24... still waiting for this...

https://media2.giphy.com/media/v1.Y2lkPTc5MGI3NjExaWpzMHl4dnVjeTVvOThweWw1anl2djFhcDJvbG5zbWJhdjhwdWFtbSZlcD12MV9pbnRlcm5hbF9naWZfYnlfaWQmY3Q9Zw/aiL06ZH64pof6/giphy.gif

@bcrochet
Copy link

bcrochet commented Feb 5, 2025

Looks like a rebase may be needed? Otherwise...

/lgtm

@dirgim dirgim force-pushed the ecosystem-bundle-validation branch from 1e615d7 to aed5e7b Compare February 5, 2025 08:28
@bcrochet bcrochet added this pull request to the merge queue Feb 5, 2025
Merged via the queue into konflux-ci:main with commit 2b9bac9 Feb 5, 2025
16 of 17 checks passed
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.

9 participants