ci: transition to github app authentication for gitops - #62
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #62 +/- ##
========================================
Coverage 97.07% 97.07%
========================================
Files 48 48
Lines 4271 4278 +7
========================================
+ Hits 4146 4153 +7
Misses 125 125
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dakotabenjamin
left a comment
There was a problem hiding this comment.
After clarity from discussion hotosm/k8s-infra#183 this is good to go. App was added and branch protection configured.
| # Use the short commit SHA as a unique, immutable tag | ||
| IMAGE_TAG="sha-${GITHUB_SHA::7}" | ||
| VALUES_FILE="k8s-infra/apps/fair-models/helm/values.yaml" | ||
| ALL_MODELS="dinov3s_buildings resnet18_classification unet_segmentation yolo11n_detection" |
There was a problem hiding this comment.
This would need to get updated as new models approved?
There was a problem hiding this comment.
Looks like it. An env var at the top of the file would be much more logical than buried here
There was a problem hiding this comment.
Yes should be dynamic !
There was a problem hiding this comment.
Great catch, guys! @spwoodcock I originally thought about pulling it up to a top-level env var, but @kshitijrajsharma is absolutely right—making it fully dynamic is the better long-term play. I’ve updated the workflow to use a find command that automatically scans the models/ directory. Now, whenever new models are added, the pipeline will detect them automatically without anyone needing to maintain a list.
| sudo chmod a+x /usr/local/bin/yq | ||
|
|
||
| - name: Login to GHCR | ||
| uses: docker/login-action@v3 |
There was a problem hiding this comment.
Good catch! Bumped docker/login-action to @v4 to keep the runner environments current and avoid any Node runtime deprecation warnings.
| ALL_MODELS="dinov3s_buildings resnet18_classification unet_segmentation yolo11n_detection" | ||
|
|
||
| # Determine which models to build | ||
| if [ "${{ github.event_name }}" == "workflow_dispatch" ]; then |
There was a problem hiding this comment.
on manual trigger , we can get teh manual input for the model_folder name and all this scanning is not needed !
There was a problem hiding this comment.
Good catch! Allowing manual input for a specific model folder during a workflow_dispatch run is a great idea. It prevents us from making unnecessary configuration updates to the whole catalog if we only need to test one model. I've updated the trigger to accept a model_name input (defaulting to 'all').
You're totally right that we don't need the scanning for targeted manual runs, so the script now bypasses it when a specific model is entered. However, I retained the git diff scanning logic for our automated runs so the pipeline continues to automatically detect and update the deployment infrastructure only for the models that actually changed during regular merges.
|
|
||
| if [ -z "$CHANGED_MODELS" ]; then | ||
| echo "No models were modified in this commit. Skipping image builds." | ||
| else |
There was a problem hiding this comment.
You can reuse this : https://github.com/hotosm/fAIr-models/blob/develop/.github/scripts/detect_models.py ? perhaps ?
There was a problem hiding this comment.
That is a great idea to keep things dry. I looked into reusing detect_models.py, but it looks like get_changed_models() is currently hardcoded to return None if the EVENT_NAME isn't a pull_request.
Since our deployment workflow triggers on push (to develop/staging/main) and workflow_dispatch, using the script as-is would bypass the diff check and build all models on every merge. It also wouldn't natively handle the manual input target I just set up.
Would you be okay with sticking to the current Bash implementation for this PR so we can get the infrastructure unblocked? We can easily swap it out in the future if detect_models.py is updated to support push events and manual model arguments!
| # 1. Infer Image HREF using your STAC script | ||
| IMAGE_HREF=$(uv run python scripts/stac_asset.py "models/${MODEL_NAME}/stac-item.json" mlm:inference) | ||
|
|
||
| # 2. Build the Docker Image |
There was a problem hiding this comment.
No need , it doesn't have to be build here , we already have model build section this workflow can depend on that workflow and only execute this one after that so you can set dependent https://stackoverflow.com/questions/58457140/dependencies-between-workflows-on-github-actions
There was a problem hiding this comment.
Great call! I've updated the deployment workflow to drop the heavy Docker build steps and use on: workflow_run so it triggers immediately after Build Model Images completes successfully. This makes it much cleaner!
One quick thing though: I noticed build-model-images.yml isn't currently tagging the inference images with the short Git SHA. Could we add type=sha,format=short to the docker/metadata-action tags in your workflow? This CD pipeline relies on those immutable sha-* tags to securely track and update ArgoCD.
There was a problem hiding this comment.
Good call, we want the sha.
The latest gh-workflows do this 👍
|
Apologies for missing the last call / context, and being late replying. I'm really fighting the urge to babysit this & hold things up 😅 But a workflow that directly commits to our core infra repo seems a bit sus to me. Perhaps it can be well justified if I dug into the implementation and plan a bit more. I just have two questions and I'll leave this alone:
|
|
Hey Sam, no worries at all! Your concern is completely valid and I appreciate you keeping an eye on the infra repo's security. To answer your questions: 1. Have we considered raising a PR instead? 2. What other approaches did we consider?
Let me know how you'd like to proceed! I can swap the |
|
Thanks for clarifying! How much lag time would using Argo image updater introduce? Would this be an unacceptable compromise? Considering, as you say, this pattern is used elsewhere, is well understood, and removes the need to push to the repo. Otherwise, if this approach is deemed the best, then we should also assess: how frequently will this be done on dev? Does a split workflow for dev/stage make sense considering how much time will be saved? |
|
Hey Sam, you make a great point. To answer your question: the lag time with the argocd-image-updater would just be its standard polling interval (around 2-3 minutes). For our model development lifecycle on dev/stage, a 3-minute delay is a completely acceptable compromise. You are entirely right that reusing the pattern we already established (like we have for kube-ops-view) keeps our stack standardized and completely eliminates the need to provision cross-repo write tokens. The minor time saved by a push-based webhook isn't worth the extra technical debt and security considerations. I'll start working on the argocd-image-updater solution, but I won't scrap the deploy-knative.yml script just yet. There's a slight implementation catch I need to work through first: because our single Helm chart manages multiple models, I have to figure out the exact annotation mapping so ArgoCD knows how to tie each unique model image alias to its specific nested Helm parameter. (As a quick FYI, I'm also going to ask Kshitij to add short Git SHA tags to his build-model-images.yml workflow, since the Image Updater needs unique, immutable tags to properly detect new builds instead of relying on the rolling latest tags). It's going to take me a little time to get the configuration right, but I'll submit a PR to the k8s-infra repo tomorrow for you to check out. If you're good with how that looks, we can close this PR. Thanks for pushing back on this and steering it in a cleaner direction! |
|
@kshitijrajsharma Thanks for the advice on chaining the workflows! Following some architectural discussions with Sam, we've actually considering to drop the custom deployment script entirely. Instead, we are going to handle this natively by adding ArgoCD Image Updater annotations directly to the fair-models manifest in the k8s-infra repo. This keeps everything standard and pull-based! One quick thing we still need from your end, though: I noticed build-model-images.yml isn't currently tagging the inference images with the short Git SHA. Could we add type=sha,format=short to the docker/metadata-action tags in your workflow? ArgoCD Image Updater relies heavily on those unique, immutable sha-* tags to cleanly detect new builds and trigger the deployments. |
|
Can we use the reusable workflows from hotosm/gh-workflows? The latest v4.x.x workflows include a sha in the tags |
|
Good call, @spwoodcock! Switching to the hotosm/gh-workflows reusable workflows is definitely the best move here to standardize the CI and get those SHA tags for free. Just a quick heads-up based on our latest Slack sync, though: we are actually pivoting away from the ArgoCD Image Updater approach for the models entirely. We're going to treat the Django API as a lightweight operator to deploy the Knative services directly in-cluster. That being said, having the immutable sha-* tags from the reusable workflow is still exactly what we need. When a user registers a model, the Django API will need a specific, immutable tag to lock into the Knative manifest anyway. @kshitijrajsharma, using Sam's suggestion for the reusable workflow sounds like a win-win for generating the images if you want to implement that! |
Overview
This PR refactors our Knative deployment workflow to act strictly as a Continuous Deployment (CD) pipeline. Following some great team feedback, I've chained this workflow to run after the
Build Model Imagespipeline finishes. This stops us from duplicating Docker builds, saving us CI minutes and preventing potential race conditions.It still handles the GitOps side of things by scanning for changes and securely updating the
values.yamlin ourk8s-infrarepository with the newly built immutable image tags.Changes Made
workflow_runso it executes automatically upon the successful completion of theBuild Model Imagespipeline. Removed alldocker build,push, andloginsteps since the upstream workflow already handles the heavy lifting.findcommand. If a data scientist adds a new model directory undermodels/, the pipeline will detect and process it automatically without requiring maintenance.model_nameinput to theworkflow_dispatchtrigger. When testing manually, you can now target a specific model (e.g.,unet_segmentation) instead of forcing the script to process the entire catalog. Defaults toall.develop,staging,main) in thek8s-infrarepository based on the triggering branch.actions/create-github-app-tokenstep to generate a short-lived token using our dedicated internal GitHub App.Testing Notes
INFRA_APP_ID/INFRA_APP_PRIVATE_KEYsecrets to this repo.workflow_dispatchin the Actions tab once the credentials are live. The manual trigger will pop up a text box asking which model you want to update.