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

[WIP] Add WF for appending additional images #25

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

Conversation

Bobbins228
Copy link

@Bobbins228 Bobbins228 commented Jul 22, 2024

Description of the change

Added a workflow which appends the image list in rhoai-additional-images with the latest release of the fms-hf-tuning image and creates an automated Pull Request after successful tests are performed.

This workflow will also push a rhoai-xxx tagged version of the working release image.

Related issue number

Closes: RHOAIENG-9937

How to verify the PR

Run the Workflow in Github actions.
After specifying the version of RHOAI you want. A PR should be made to rhoai-additional-images with the latest release image SHA appended to the image list of the given rhoai-xxx branch.

A rhoai-xxx tag of the release image should be pushed to modh/fms-hf-tuning in Quay.

TODO

Add CF Machine account credentials to this repo
Add Quay credentials to this repo
Add testing step to the workflow

Note: you can run this WF with gh workflow run fms-additonal-image-push.yaml -R github.com/redhat-data-services/fms-hf-tuning -f rhoai-release-version=2.xx

Was the PR tested

I have tested this workflow on my forked repositories and private quay repo.

  • I have added >=1 unit test(s) for every new method I have added.
  • I have ensured all unit tests pass

Copy link

@KPostOffice KPostOffice left a comment

Choose a reason for hiding this comment

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

Looks good. A few comments

Comment on lines +39 to +41
- name: Perform tests
run: |
echo "Performing functional tests"

Choose a reason for hiding this comment

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

This can be removed right?

Copy link
Author

Choose a reason for hiding this comment

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

For now this is just a place holder until we can set up some proper tests.

Comment on lines +46 to +47
git config --global user.email "[email protected]"
git config --global user.name "codeflare-machine-account"

Choose a reason for hiding this comment

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

small nit, can we put these in the env vars too?

Choose a reason for hiding this comment

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

Or as repo variables maybe?

git config --global user.name "codeflare-machine-account"
git checkout -b $UPDATE_BRANCH
# Update the image list to include the fms-hf-tuning release image
export IMAGE_SHA=$(podman pull --quiet quay.io/$IMAGE_BASE/$IMAGE_REPO:release)

Choose a reason for hiding this comment

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

Is there any way to do this without pulling the whole image? The image is 3.3Gb which is relatively large.

git add .
git commit -am "Updated rhoai-disconnected-images.yaml with latest fms-hf-tuning release image" --signoff
git push origin $UPDATE_BRANCH
git remote set-url origin https://x-access-token:${GITHUB_TOKEN}@github.com/$REPO_OWNER/$REPO_NAME.git

Choose a reason for hiding this comment

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

What is this line doing? Is this so that the WF is authenticated when creating the PR?

Copy link
Author

Choose a reason for hiding this comment

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

Yes it is for authentication purposes for the gh create pr command

Choose a reason for hiding this comment

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

Can you move it down to that step just so they are linked?

Comment on lines +78 to +79
podman tag quay.io/$IMAGE_BASE/$IMAGE_REPO:release quay.io/$IMAGE_BASE/$IMAGE_REPO:$RHOAI_BRANCH
podman push quay.io/$IMAGE_BASE/$IMAGE_REPO:$RHOAI_BRANCH

Choose a reason for hiding this comment

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

Suggested change
podman tag quay.io/$IMAGE_BASE/$IMAGE_REPO:release quay.io/$IMAGE_BASE/$IMAGE_REPO:$RHOAI_BRANCH
podman push quay.io/$IMAGE_BASE/$IMAGE_REPO:$RHOAI_BRANCH
podman push quay.io/$IMAGE_BASE/$IMAGE_REPO:release
quay.io/$IMAGE_BASE/$IMAGE_REPO:$RHOAI_BRANCH

I think this should work with just a single line

Copy link
Author

Choose a reason for hiding this comment

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

Wow that's really cool I didn't know you could do that! Thanks

@riprasad riprasad force-pushed the main branch 5 times, most recently from 82372de to 5a99925 Compare September 11, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants