Kubeflow sync release pipeline #469
Conversation
…nd also to automerge or not the changes
…anch and changed its name
…t untill be ready
|
Skipping CI for Draft Pull Request. |
|
[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 |
| @@ -0,0 +1,24 @@ | |||
| --- | |||
There was a problem hiding this comment.
Hey @andyatmiami! This is only a mockup gha, it prints a Eyo, World for now 🤣 It used to work on the logic of the kubeflow release.
There was a problem hiding this comment.
So, fwiw... and I'm happy to change what I did with the create release PoC I did.. but I was condititioning the release creation logic on the push of a tag matching a specific pattern...
So it wouldn't need to be explicitly invoked here.. but if that "trigger off tag" won't work for reasons I am not appreciating - I am happy to adapt!
|
Looks good. I would not mind merging this even with the placeholder Hello World create release part. Submitting the GitHub release is just a cherry on top. |
|
/test odh-notebook-controller-e2e |
|
/lgtm |
atheo89
force-pushed
the
RHOAIENG-15453
branch
from
2db292a
to
5b9d718
Compare
|
/override ci/prow/odh-notebook-controller-e2e |
|
@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/odh-notebook-controller-e2e 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. |
|
/lgtm |
atheo89
force-pushed
the
RHOAIENG-15453
branch
from
36e6b4b
to
756fa2c
Compare
atheo89
force-pushed
the
RHOAIENG-15453
branch
from
756fa2c
to
1c04397
Compare
|
/override ci/prow/odh-notebook-controller-e2e |
|
@atheo89: Overrode contexts on behalf of atheo89: ci/prow/odh-notebook-controller-e2e 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. |
|
/lgtm |
| # PR to look for | ||
| PR_TITLE="[GHA-${{ github.run_id }}]" | ||
| # Fetch matching PRs | ||
| gh pr list --repo atheo89/kubeflow --state all --search "$PR_TITLE" --json number,title > pr_list.json |
There was a problem hiding this comment.
| gh pr list --repo atheo89/kubeflow --state all --search "$PR_TITLE" --json number,title > pr_list.json | |
| gh pr list --repo "$REPO_OWNER/kubeflow" --state all --search "$PR_TITLE" --json number,title > pr_list.json |
There was a problem hiding this comment.
We can also use the --jq option on the gh CLI call to avoid needing to dump this to file. And while presently we specify in --json to return the title - I don't see us actually using that anywhere..
| for (( i=1; i<=MAX_ATTEMPTS; i++ )); do | ||
| echo "Checking if PR #$PR_NUMBER is merged (Attempt $i/$MAX_ATTEMPTS)..." | ||
| PR_STATE=$(gh pr view --repo atheo89/kubeflow $PR_NUMBER --json mergedAt --jq '.mergedAt') |
There was a problem hiding this comment.
| PR_STATE=$(gh pr view --repo atheo89/kubeflow $PR_NUMBER --json mergedAt --jq '.mergedAt') | |
| PR_STATE=$(gh pr view --repo "$REPO_OWNER/kubeflow" $PR_NUMBER --json mergedAt --jq '.mergedAt') |
There was a problem hiding this comment.
If --jq is specified.. I don't think the --json argument does anything... (but I could be wrong...)
| steps: | ||
| - name: Poll for images availability | ||
| id: check-images | ||
| run: | |
There was a problem hiding this comment.
Again, personal preference here (so fine to ignore) - but if we are going to inline scripts that aren't a couple lines... pulling that out into a proper external script seems easier to work with (i.e. don't need to worry about the lurking concern of YAML indentation, etc)
However, doing so requires the actions/checkout@v4 workflow.. and I'm not clear on the potential performance concerns (if any) of introducing that.
- although i notice we do leverage this workflow later in this file:
| @@ -68,12 +97,12 @@ jobs: | |||
| - name: Update related files | |||
| id: apply-changes | |||
| run: | | |||
There was a problem hiding this comment.
Logic of this complexity could be warranted to pull out into a separate script for ease of maintenance (just a suggestion!)
| git config user.email "github-actions[bot]@users.noreply.github.com" | ||
|
|
||
| - name: Merge source branch into target | ||
| run: | |
There was a problem hiding this comment.
Pull this logic out into a separate script to be invoked (?)
|
Nice job.. even a "relative newcomer" like me could follow the logical flow through the work 💯 I added various comments through the PR - but nothing that I would consider a blocking issue that:
so... /lgtm |
|
ℹ️ On one last quick scan through the code.. it occurred to me that in a couple places we have hard-coded the Given we seem to be reading input for some of these jobs already... might be good to think about how we can parameterize this value to keep the script logic version-agnostic. However, that can certainly just be handled as technical debt. |
Related to: https://issues.redhat.com/browse/RHOAIENG-15453
Description
This PR incorporates the idea to automate completely the kubeflow release procedure
Key Points:
sync-brances.yamlupdated to be compatible as callable action from another. Additionally, ensures that changes in the main branch are reflected in the release branch.create-relase.yamlis just a mockup in order to go on with the dev, This workload will be developed through RHOAIENG-15391Investigation Item:
Investigate how the PR generated from the
update-release-imagesstep can be auto-merged in case all the tests pass, without user innervation. Currently, needs user approval in order to go the the last step.How Has This Been Tested?
Tested locally:
Run: https://github.com/atheo89/kubeflow/actions/runs/12007990443
Video Demo:
With set
create-new-releasetrue & falsehttps://drive.google.com/file/d/1uXC5xuTvoR9FWvfMYs2H13zIHa_Usrps/view?usp=sharing
Merge criteria: