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

RHOAIENG-18400: chore(konflux): create helper scripts to generate konflux configs #903

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

Conversation

jiridanek
Copy link
Member

@jiridanek jiridanek commented Feb 18, 2025

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:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@openshift-ci openshift-ci bot requested review from atheo89 and dibryant February 18, 2025 12:17
Copy link
Contributor

openshift-ci bot commented Feb 18, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jiridanek. For more information see the Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jiridanek
Copy link
Member Author

/override ci/prow/images
n/a for this pr

Copy link
Contributor

openshift-ci bot commented Feb 18, 2025

@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/images

In response to this:

/override ci/prow/images
n/a for this pr

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.

@jiridanek jiridanek changed the title NO-JIRA: chore(konflux): create helper scripts to generate konflux configs RHOAIENG-18400: chore(konflux): create helper scripts to generate konflux configs Feb 18, 2025
@openshift-ci openshift-ci bot removed the size/xxl label Feb 18, 2025
Copy link
Contributor

@andyatmiami andyatmiami left a 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 = []
Copy link
Member Author

@jiridanek jiridanek Feb 18, 2025

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.

Copy link
Member Author

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.

Copy link
Contributor

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

Copy link
Member Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants