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.

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

Jump to bottom

feat(KONFLUX-3935) Add pruning check to FBC pipeline #1744

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(KONFLUX-3935) Add pruning check to FBC pipeline #1744

wants to merge 1 commit into from
+319 −3

Conversation

nmars
Copy link

@nmars nmars commented Dec 10, 2024

Adds a new task to the fbc-builder pipeline to check if the incoming FBC fragment would remove channels or channel entries already present in the target index.

@nmars nmars requested review from a team as code owners December 10, 2024 21:32
@nmars nmars force-pushed the add-fbc-pruning-check branch 2 times, most recently from ad0b49a to 43f2f6c Compare December 11, 2024 17:42
MartinBasti
MartinBasti previously approved these changes Dec 11, 2024
View reviewed changes
Copy link
Contributor

@MartinBasti MartinBasti left a comment

Choose a reason for hiding this comment

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

Approving CODEOWNERS change only

dirgim
dirgim previously approved these changes Dec 18, 2024
View reviewed changes
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 code-wise but needs rebase and resolving the CODEOWNERS conflict

@nmars nmars dismissed stale reviews from dirgim and MartinBasti via 378c245 January 2, 2025 19:25
@nmars nmars force-pushed the add-fbc-pruning-check branch from 43f2f6c to 378c245 Compare January 2, 2025 19:25
@nmars nmars force-pushed the add-fbc-pruning-check branch 2 times, most recently from a7a0b2f to f519f1b Compare January 10, 2025 16:02
chmeliik
chmeliik previously approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

build-owned parts LGTM

dirgim
dirgim previously approved these changes Jan 10, 2025
View reviewed changes
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 code-wise

hongweiliu17
hongweiliu17 previously approved these changes Jan 13, 2025
View reviewed changes
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

@dirgim dirgim force-pushed the add-fbc-pruning-check branch from f519f1b to 06b085e Compare January 13, 2025 08:50
@dirgim
Copy link
Contributor

dirgim commented Jan 13, 2025

/ok-to-test

@dirgim
Copy link
Contributor

dirgim commented Jan 13, 2025

/retest

@nmars nmars force-pushed the add-fbc-pruning-check branch from 06b085e to b9447a0 Compare January 13, 2025 17:11
@dirgim
Copy link
Contributor

dirgim commented Jan 14, 2025

/retest

arewm
arewm requested changes Jan 14, 2025
View reviewed changes
Copy link
Member

@arewm arewm left a comment

Choose a reason for hiding this comment

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

Is there specific benefit to creating an additional task for this check instead of just including it in the current validate-fbc task?

By creating multiple tasks, we have to perform similar actions twice like fetching/rendering the FBC catalog. I had talked with @yashvardhannanavati about how it would be nice if the functions in the konflux-test image could reuse a common shared cache of the catalog. Separating the FBC verification checks into multiple tasks would invalidate benefits achieved with this approach.

@nmars
Copy link
Author

nmars commented Jan 14, 2025

Is there specific benefit to creating an additional task for this check instead of just including it in the current validate-fbc task?

By creating multiple tasks, we have to perform similar actions twice like fetching/rendering the FBC catalog. I had talked with @yashvardhannanavati about how it would be nice if the functions in the konflux-test image could reuse a common shared cache of the catalog. Separating the FBC verification checks into multiple tasks would invalidate benefits achieved with this approach.

@arewm The benefit of having a separate check is because we want to be able to configure it for one-off exceptions. This is the proposed way to deal with the rare instances where a team needs to remove a version or channel from the target index. But they would want to ensure the other checks in validate-fbc would pass.

@nmars nmars dismissed stale reviews from dirgim, hongweiliu17, and chmeliik via da8674e January 17, 2025 14:43
@nmars nmars force-pushed the add-fbc-pruning-check branch from b9447a0 to da8674e Compare January 17, 2025 14:43
@dirgim
Copy link
Contributor

dirgim commented Jan 17, 2025

/ok-to-test

@nmars nmars force-pushed the add-fbc-pruning-check branch from da8674e to 9f757fe Compare January 17, 2025 14:46
@dirgim
Copy link
Contributor

dirgim commented Jan 17, 2025

/ok-to-test

nmars
nmars commented Jan 17, 2025
View reviewed changes
pipelines/fbc-builder/README.md
@@ -189,8 +201,10 @@ This pipeline is pushed as a Tekton bundle to [quay.io](https://quay.io/reposito
|name|description|used in params (taskname:taskrefversion:taskparam)
|---|---|---|
|IMAGES_PROCESSED| Images processed in the task.| |
|OCP_VERSION| OCP version derived from base image.| fbc-target-index-pruning-check:0.1:OCP_VERSION|
|RELATED_IMAGES_DIGEST| Digest for attached json file containing related images| |
|RELATED_IMAGE_ARTIFACT| The Trusted Artifact URI pointing to the artifact with the related images for the FBC fragment.| |
Copy link
Author

Choose a reason for hiding this comment

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

@arewm How does RELATED_IMAGE_ARTIFACT get created?

Copy link
Author

Choose a reason for hiding this comment

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

Latest change: the validate-fbc task was modified to create a trusted artifact from the opm render output. It will be pulled and used in the new check in lieu of running opm render again.

@nmars nmars force-pushed the add-fbc-pruning-check branch from 9f757fe to 27c6fc7 Compare January 17, 2025 16:00
@dirgim dirgim force-pushed the add-fbc-pruning-check branch from 27c6fc7 to 846f3b5 Compare January 20, 2025 10:10
@dirgim
Copy link
Contributor

dirgim commented Jan 20, 2025

/ok-to-test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arewm arewm arewm requested changes

@chmeliik chmeliik chmeliik left review comments

@hongweiliu17 hongweiliu17 hongweiliu17 left review comments

@dirgim dirgim dirgim left review comments

@MartinBasti MartinBasti MartinBasti left review comments

@14rcole 14rcole Awaiting requested review from 14rcole

@Josh-Everett Josh-Everett Awaiting requested review from Josh-Everett

@kasemAlem kasemAlem Awaiting requested review from kasemAlem

@dheerajodha dheerajodha Awaiting requested review from dheerajodha

@sonam1412 sonam1412 Awaiting requested review from sonam1412

@jsztuka jsztuka Awaiting requested review from jsztuka

@jencull jencull Awaiting requested review from jencull

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nmars @dirgim @arewm @MartinBasti @hongweiliu17 @chmeliik @openshift-merge-robot