-
Notifications
You must be signed in to change notification settings - Fork 90
[RHOAIENG-24973] [RHOAIENG-26615] Created notebooks-digest-updater.yaml and update-buildconfigs.yaml to automate the release of notebooks with the github action call: Notebooks Release #1150
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
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 |
Hi @mtchoum1. Thanks for your PR. I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/ok-to-test |
1e611de
to
f277c3d
Compare
f277c3d
to
6614bc6
Compare
cat tmp.yaml > ./manifests/base/cuda-rstudio-buildconfig.yaml | ||
rm tmp.yaml | ||
|
||
- name: Update BuildConfig for RStudio |
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.
Just wondering why not to keep both rstudio updates in one step? Not a big deal though.
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.
It could be done, I did it just so I knew both we're completed
jobs: | ||
buidConfigs: | ||
runs-on: ubuntu-latest | ||
if: inputs.version == '' |
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.
I wonder whether instead of having different job for each variant meaning having the duplicate code, we couldn't have an extra step that would define a new variable depending on the inputs.version
value - either use it directly or count a new value for us. This new variable would then be used by the followup steps. WDYT?
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.
That's an excellent idea! I'll definitely try to implement that.
|
||
env: | ||
USER_HASH: ${{ github.event.inputs.user-hash }} | ||
USER_HASH: ${{ inputs.user-hash }} |
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.
Can you elaborate on why this change? Is it because of the added workflow_call
option above? But I wonder why it wasn't a problem for the workflow_dispatch
then 🤔 or is it because of the external call from the notebook-release.yaml
?
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.
Yes, From my understanding and testing if you use workflow_call you have to use input.user-hash instead of the github.event.input.userhash if there's external call
|
||
if [ "$PR_STATE" = "null" ] || [ -z "$PR_STATE" ]; then | ||
echo "PR #$PR_NUMBER is not merged yet. Waiting..." | ||
sleep 1 |
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.
Hm, I'm not sure I like the waiting step. At least we should probably sleep for more than 1 second.
Anyway, how do we expect the manifest PR to be merged? Is it gonna be automatically as we discussed last week?
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.
Anyway, how do we expect the manifest PR to be merged? Is it gonna be automatically as we discussed last week?
By hand I figure, same as in Kubeflow repo. Until something more is implemented.
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.
Yeah, my expectation was that we plan to wait for the odh on konflux.
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.
Quite likely, that's why I was complaining about refining the automerge issue right now, since whatever we decide will be invalidated by the time we end up implementing.
Anyways, these concerns don't affect this PR at all, 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.
Thank for this work Moryan, I am adding a few suggestions that could help reduce redundancy and improve maintainability in update-buildconfig.yaml
workflow:
There's a lot of repeated code between the two jobs (buidConfigs and buidConfigs_with_version).
It would be cleaner to extract the logic into a reusable bash script (e.g., buildconfig-updater.sh) and call it with the version as an argument:
./build-config-updater.sh ${{ env.VERSION }}
Inside the script, you can handle version logic conditionally:
if [[ -z "$VERSION" ]]; then
# Auto-increment version logic
else
# Use provided version logic
fi
Moreover, as Jan pointed out, there's no need to have separate steps for updating cuda-rstudio and rstudio buildconfigs. If the above suggestion isn’t followed, at the very least, these two very similar blocks could be merged into one to reduce duplication.
Lastly, please update the pull request title and description to reference both related issues: RHOAIENG-24975
and RHOAIENG-24973
. I'd recommend keeping the scope of each workflow or pull request as small as reasonably possible. Smaller, focused changes generally move through review quicker and make life easier for both developers and reviewers.
Thanks!
4056aca
to
7f4189c
Compare
…thub action call: Notebooks Release
7f4189c
to
77215f4
Compare
…hin the job rather that for the job itself and changed the merge check to be the same as kubeflow
@@ -0,0 +1,206 @@ | |||
--- |
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.
Could you add two options to this GitHub Action?
Release
: This should work as it currently does, including all jobs (digest updater, build config, release etc.).
Sync
: This should trigger only the digest updater. The release and build config jobs should not be triggered.
The Sync
option is important to ensure that params-latest.env stays regularly updated. This will support future plans to use ODH nightlies and, potentially, set up a daily cron job for automatic syncing. The Release
option would then be used only on demand for actual releases.
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.
Yes, They have something similar in kubeflow release, I can replicate what they have done
VERSION: ${{ github.event.inputs.buildconfigs_version}} | ||
|
||
jobs: | ||
#1. Update the params.env and commit.env files with new SHAs |
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.
#1. Update the params.env and commit.env files with new SHAs | |
#1. Update the params-latest.env and commit-latest.env files with new SHAs |
3973e88
to
3e93e70
Compare
8cb77d3
to
8767783
Compare
… automate the release of notebooks with the github action call: Notebooks Release
8767783
to
712e4da
Compare
Automated Notebooks Releases
Description
[RHOAIENG-24973]
.github/workflows/notebooks-release.yaml
: Created to facilitate notebook releases by calling.github/workflows/notebooks-digest-updater.yaml
and.github/workflows/update-buildconfigs.yaml
. This workflow allows users to optionally update build configurations and input the release version and name.[RHOAIENG-26615]
.github/workflows/update-buildconfigs.yaml
: Created to update build configuration files in the manifest. This workflow can be run independently or called by the notebook release workflow. You can input a version; if left blank, it will simply increment the last version by one.How Has This Been Tested?
[RHOAIENG-24973]
By running the GitHub Action on my own repository using the branch tmp-digest-sync-14488482962 to simulate the output of
.github/workflows/notebooks-digest-updater.yaml
[RHOAIENG-26615]
By running the Github Action on my own repository checking the pull request created to make sure the version was changing correct based on the input given. Also tested it being called by the
.github/workflows/update-buildconfigs.yaml
Merge criteria: