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
+320 −4

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

@nmars nmars force-pushed the add-fbc-pruning-check branch from 846f3b5 to 3879925 Compare January 27, 2025 18:52
@dirgim
Copy link
Contributor

dirgim commented Jan 28, 2025

/ok-to-test

@nmars nmars requested a review from arewm January 29, 2025 15:03
@nmars nmars force-pushed the add-fbc-pruning-check branch from 3879925 to 879c006 Compare January 29, 2025 16:03
@dirgim
Copy link
Contributor

dirgim commented Jan 29, 2025

/ok-to-test

@dirgim
Copy link
Contributor

dirgim commented Jan 30, 2025

/retest

@nmars nmars force-pushed the add-fbc-pruning-check branch from 879c006 to 0b098e0 Compare January 30, 2025 14:19
@dirgim
Copy link
Contributor

dirgim commented Jan 30, 2025

/ok-to-test

@nmars nmars force-pushed the add-fbc-pruning-check branch from 0b098e0 to a3e8bb3 Compare January 30, 2025 18:24
yashvardhannanavati
yashvardhannanavati reviewed Jan 30, 2025
View reviewed changes
task/validate-fbc/0.1/README.md Outdated
@@ -16,4 +16,5 @@ Ensures file-based catalog (FBC) components are uniquely linted for proper const
|TEST_OUTPUT|Tekton task test output.|
|RELATED_IMAGES_DIGEST|Digest for attached json file containing related images|
|IMAGES_PROCESSED|Images processed in the task.|
|OCP_VERSION|OCP version of FBC image.|
Copy link
Contributor

Choose a reason for hiding this comment

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

Should RENDERED_CATALOG_DIGEST be added here too?

Also, could you have the changes to validate-fbc in a separate commit?

An afterthought. We should be using konflux-test helper functions to get OCP_VERSION and rendered catalog. Tekton results have a size limit and this approach wouldn't be scalable.

yashvardhannanavati
yashvardhannanavati approved these changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@yashvardhannanavati yashvardhannanavati left a comment

Choose a reason for hiding this comment

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

one non-blocking comment. looks good

@nmars nmars force-pushed the add-fbc-pruning-check branch from a3e8bb3 to 581f6b8 Compare January 31, 2025 14:14
@dirgim
Copy link
Contributor

dirgim commented Jan 31, 2025

/ok-to-test

@konflux-ci-qe-bot
Copy link

@dirgim: The following test has Completed, say /retest to rerun failed tests.

PipelineRun Name Status Rerun command Build Log Test Log
build-definitions-pull-request-8fpkf Completed /retest View Pipeline Log View Test Logs

Inspecting Test Artifacts

To inspect your test artifacts, follow these steps:

  1. Install ORAS (see the ORAS installation guide).
  2. Download artifacts with the following commands:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/konflux-test-storage/konflux-team/build-definitions:581f6b8d98f73e6537b3d1dbf5a28134950a33ed

Test results analysis

🚨 Error occurred while running the E2E tests, list of failed Spec(s):

@nmars nmars force-pushed the add-fbc-pruning-check branch from 581f6b8 to 1f9b357 Compare January 31, 2025 16:52
@nmars nmars force-pushed the add-fbc-pruning-check branch from 1f9b357 to 5a37754 Compare January 31, 2025 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yashvardhannanavati yashvardhannanavati yashvardhannanavati approved these 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

@arewm arewm Awaiting requested review from arewm

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.
9 participants
@nmars @dirgim @konflux-ci-qe-bot @arewm @MartinBasti @yashvardhannanavati @hongweiliu17 @chmeliik @openshift-merge-robot