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

Generate Konflux pipelines #163

Conversation

pierDipi
Copy link
Member

@pierDipi pierDipi commented Jun 18, 2024

Follow up to #161

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
@openshift-ci openshift-ci bot requested review from mgencur and skonto June 18, 2024 09:50
@pierDipi
Copy link
Member Author

/cc @mgencur @creydr

@openshift-ci openshift-ci bot requested a review from creydr June 18, 2024 09:50
@mgencur
Copy link
Contributor

mgencur commented Jun 18, 2024

I haven't fully reviewed this but I can see there's just one type of Pipeline. I was working on openshift-knative/serverless-operator#2707 to build all images there and figured there are two Pipelines that are slightly different. I call them serverless-image and serverless-index (the Pipeline for the index image is slightly different). Maybe @creydr could shed some light on the difference as I just copied them from the original pipelines.
The index image is probably not considered in this PR. Just something to think about for the future.

@creydr
Copy link
Member

creydr commented Jun 18, 2024

I haven't fully reviewed this but I can see there's just one type of Pipeline. I was working on openshift-knative/serverless-operator#2707 to build all images there and figured there are two Pipelines that are slightly different. I call them serverless-image and serverless-index (the Pipeline for the index image is slightly different). Maybe @creydr could shed some light on the difference as I just copied them from the original pipelines. The index image is probably not considered in this PR. Just something to think about for the future.

The serverless-index pipeline is a bit different, because it has additional steps to verify the file based catalog of the index. This was mostly auto generated, as Konflux recognized it as an index build (therefor I needed to reference a devfile, which I reverted, after we had the pipeline generated (openshift-knative/serverless-operator@2a6573d), as this made things more complicated and was not needed anymore (as we had the FBC related tasks in the pipeline)).

@pierDipi
Copy link
Member Author

pierDipi commented Jun 18, 2024

That pipeline will need to be special cased, it's this one basically https://github.com/konflux-ci/build-definitions/tree/main/pipelines/fbc-builder

@creydr
Copy link
Member

creydr commented Jun 18, 2024

One other thing we should keep in mind is to only trigger a pipeline on relevant changes for the component. Otherwise this could trigger endless renovate PRs. See from openshift-knative/serverless-operator#2674

Prevent endless renovate PRs as serverless-operator and serverless-bundle are in the same repository (As both are in the same repository, an update on the bundle (e.g. via an automated renovate PR to update the image references) triggers a new build of the serverless-operator. As this results in a new image tag, a new renovate PR for the CSV is sent again (-> endless loop).): openshift-knative/serverless-operator@09a1470

@pierDipi
Copy link
Member Author

But for now, I'm focusing on dependents component, SO will come next

@creydr
Copy link
Member

creydr commented Jun 18, 2024

But for now, I'm focusing on dependents component, SO will come next

ACK. Just wanted to mention it in this context, so we don't forget about it

@pierDipi
Copy link
Member Author

Let me know if the PR looks ok and code is clean/readable enough as I'd like to try it out for eventing core

Copy link
Member

Choose a reason for hiding this comment

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

Only to make sure I understand this correctly: this is the "default-custom" pipeline from Konflux?

Copy link
Member Author

@pierDipi pierDipi Jun 18, 2024

Choose a reason for hiding this comment

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

Yes, I think so

Copy link
Member Author

@pierDipi pierDipi Jun 18, 2024

Choose a reason for hiding this comment

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

Actually, it's the new one that is not using PVC for storing artifacts and should solve our issue with PV/PVC Quota

@pierDipi pierDipi requested a review from creydr June 18, 2024 14:30
Copy link
Member

@creydr creydr left a comment

Choose a reason for hiding this comment

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

LGTM from my side

/lgtm

Copy link

openshift-ci bot commented Jun 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: creydr, pierDipi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit d4db16a into openshift-knative:main Jun 18, 2024
5 checks passed
@pierDipi pierDipi deleted the konflux-generate-pipelines branch June 18, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants