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

Add GitHub Action for building and pushing UI and BFF images #446

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

Griffin-Sullivan
Copy link
Contributor

@Griffin-Sullivan Griffin-Sullivan commented Oct 1, 2024

Description

Added a GH Action for building and pushing the UI and BFF images. Reuses the build script for Model Registry server, and edits the Makefile a little bit to get everything working. I also turned off image builds for MR server when a PR is being made only for UI / BFF changes. I can remove that if we want to keep building them.

How Has This Been Tested?

Going to test the PR as on: pull_request then if it passes will update the PR to run the action on: push.

Merge criteria:

  • All the commits have been signed-off (To pass the DCO check)
  • The commits have meaningful messages; the author will squash them after approval or in case of manual merges will ask to merge with squash.
  • 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.
  • Code changes follow the kubeflow contribution guidelines.

If you have UI changes

  • The developer has added tests or explained why testing cannot be added.
  • Included any necessary screenshots or gifs if it was a UI change.
  • Verify that UI/UX changes conform the UX guidelines for Kubeflow.

@Griffin-Sullivan
Copy link
Contributor Author

My user must not have access to the creds for the kubeflow docker so I'm assuming there's no way to test this without having the bot do it on push to main? @tarilabs any ideas how I can test?

Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

My user must not have access to the creds for the kubeflow docker so I'm assuming there's no way to test this without having the bot do it on push to main? @tarilabs any ideas how I can test?

When I face similar situation @Griffin-Sullivan , I did setup DOCKERHUB_USERNAME etc in my GitHub repo (my fork) and updated IMG_ORG to point at my username in dockerhub.io

It also gives you an idea/survey of the changes we will need to apply midstream (kind request: please remind me/us when this PR will be synchronized midstream!)

edit: for a better "survey", I recommend also checking which changes are required for using quay.io instead of dockerhub: that is typically about setting one more env variable AND someIMG: in the GHA update.
This will be handy not only for midstreams but also when eventually we will publish to ghcr in addition to dockerhub for this upstream

@@ -0,0 +1,94 @@
name: Build and Push UI and BFF Images
on:
pull_request:
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate in the PR description is mentioned this on: pull_request is only for testing, but I'd rather make a comment so to make sure we don't forget before merging 😅

@Griffin-Sullivan
Copy link
Contributor Author

You can see on https://github.com/Griffin-Sullivan/model-registry/actions/runs/11147635638/job/30982430523?pr=1 that the Action is passing when I push to my own quay repo. You can find them here: https://quay.io/user/gsulliva/?tab=repos

The reason the build-and-test-image action is failing is because I'm forcing the build script to run with quay.io so once that is removed when we are ready to merge it should pass fine.

@Griffin-Sullivan Griffin-Sullivan marked this pull request as ready for review October 2, 2024 17:54
@google-oss-prow google-oss-prow bot requested a review from ederign October 2, 2024 17:55
jobs:
build-image:
runs-on: ubuntu-latest
environment: griffin-testing # TODO: REMOVE LINE BEFORE MERGING
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an extra reminder this line is here :D

@@ -3,7 +3,7 @@
set -e

# see Makefile for the IMG_ variables semantic
IMG_REGISTRY=""
IMG_REGISTRY="quay.io" # TODO: CHANGE TO EMPTY STRING BEFORE MERGE!!!
Copy link
Contributor

Choose a reason for hiding this comment

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

Another reminder :D

@alexcreasy
Copy link
Contributor

Looks good, I haven't completely groked the logic but the general tasks all look good!

@Griffin-Sullivan
Copy link
Contributor Author

Latest run is still passing on https://github.com/Griffin-Sullivan/model-registry/actions/runs/11162537114/job/31027401589?pr=1. If someone will review this I can remove all the TODOs 😄

Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

I think it's worthy to remove the TODOs

/lgtm

shell: bash
env:
IMG_REPO: ${{ env.IMG_UI_REPO }}
IMG: ${{ env.IMG_ORG }}/${{ env.IMG_UI_REPO }}
Copy link
Member

Choose a reason for hiding this comment

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

this IMG will need to be prefixed with quay.io for midstream, correct?

I'm thinking if we should just be explicit and introduce docker.io/ prefix in all GHAs 🤔
what is your team thought here?:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I discussed with @lucferbux and @alexcreasy, and the current plan for midstream is to combine the UI here with the midstream image for the dashboard using webpack. That means we don't need a separate image for Model Registry UI and it will only be used for upstream.

Copy link
Member

Choose a reason for hiding this comment

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

ack! thanks for the update

Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Oct 7, 2024
@ederign
Copy link
Member

ederign commented Oct 8, 2024

@tarilabs we need you to merge this one!

Copy link
Contributor

@alexcreasy alexcreasy left a comment

Choose a reason for hiding this comment

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

All looks good to me.

@alexcreasy
Copy link
Contributor

@tarilabs would you mind approving this, if you're happy with the changes? It's outside my or Eder's jurisdiction :)

Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexcreasy, ederign, tarilabs

The full list of commands accepted by this bot can be found here.

The pull request process is described 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

@alexcreasy
Copy link
Contributor

thanks @tarilabs

@google-oss-prow google-oss-prow bot merged commit 222194a into kubeflow:main Oct 8, 2024
16 checks passed
tarilabs added a commit to tarilabs/model-registry that referenced this pull request Oct 15, 2024
Signed-off-by: Matteo Mortari <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants