-
Notifications
You must be signed in to change notification settings - Fork 75
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
RHOAIENG-18400: chore(konflux): create helper scripts to generate konflux configs #903
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/override ci/prow/images |
@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/images In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is absolutely wonderful work to see working - I'm not sure of the benefit of merging this now.... when we have changes that remove chained builds looming...
Seems like this (imho) should be coordinated along with the feature branch we have as ultimately its working against that version of the code that matters...
But certainly not a stance I feel strongly about... just another change I will need to account for as part of getting my work merged... 🤷
deps.append(dep) | ||
frontier.extend(tree[dep]) | ||
|
||
build_container_tasks = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is absolutely wonderful work to see working - I'm not sure of the benefit of merging this now.... when we have changes that remove chained builds looming...
@andyatmiami Well, that just means we would not need an array here, because there will be only a single build_container task. So the for-loop below that's to populate the array can go away too.
I'd be quite happy getting all this working for the refactor/
branch too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need this generating script, because we want to have essentially the same component-pipeline yamls for every image we have.
So it's just tweaks going forward, the overall approach still works imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, totally agree here w.r.t the overall approach being a good thing ...
Was moreso just ( lightly ) probing whether reviewing this now is the best use of time... as having this working (for instance) tomorrow with chained builds doesn't buy us a whole lot... and we have weeks before we will actually be onboarded into Konflux.
But, to your point - knowing/being comfortable with the core functionality of the generator scripts will apply now and in the future.
Personally, I'd love to do a proper review here - but i'm underwater on the Konflux refactor and need to get that wrapped up before EOD Thursday...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was moreso just ( lightly ) probing whether reviewing this now is the best use of time... as having this working (for instance) tomorrow with chained builds doesn't buy us a whole lot... and we have weeks before we will actually be onboarded into Konflux.
ODH-io has been onboarded today, https://redhat-internal.slack.com/archives/C05NXTEHLGY/p1739803343884249
Personally, I'd love to do a proper review here - but i'm underwater on the Konflux refactor and need to get that wrapped up before EOD Thursday...
I don't think you can get it reviewed and merged by EOD Thursday. We'll have at least one more sprint to play with all this, both your refactor, and my konflux stuff...
Yep, totally agree here w.r.t the overall approach being a good thing ...
That's all I hoped for, at this stage. Thanks for taking a (brief) look!
https://issues.redhat.com/browse/RHOAIENG-18400
Description
I took the automatic onboarding PR from Konflux as a basis, you can see it here
Created a Python script that can generate such Tekton pipeline for each of our images. For now I am only committing one such pipeline, for jupyter datascience image.
How Has This Been Tested?
Merge criteria: