Skip to content

ci: transition to github app authentication for gitops - #62

Open
yshmael wants to merge 7 commits into
hotosm:developfrom
yshmael:feature/knative-gitops
Open

ci: transition to github app authentication for gitops#62
yshmael wants to merge 7 commits into
hotosm:developfrom
yshmael:feature/knative-gitops

Conversation

@yshmael

@yshmael yshmael commented Aug 3, 2026

Copy link
Copy Markdown

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 Images pipeline 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.yaml in our k8s-infra repository with the newly built immutable image tags.

Changes Made

  • Separated CI and CD: Switched the trigger to workflow_run so it executes automatically upon the successful completion of the Build Model Images pipeline. Removed all docker build, push, and login steps since the upstream workflow already handles the heavy lifting.
  • Dynamic Model Discovery: Replaced the hardcoded list of models with a dynamic find command. If a data scientist adds a new model directory under models/, the pipeline will detect and process it automatically without requiring maintenance.
  • Targeted Manual Runs: Added a model_name input to the workflow_dispatch trigger. 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 to all.
  • Multi-Environment Ready: Dynamically targets the correct environment branch (develop, staging, main) in the k8s-infra repository based on the triggering branch.
  • Secure Cross-Repo Auth: Integrates the actions/create-github-app-token step to generate a short-lived token using our dedicated internal GitHub App.

Testing Notes

  • DO NOT MERGE YET: Waiting for DK to finish provisioning the GitHub App and adding the INFRA_APP_ID / INFRA_APP_PRIVATE_KEY secrets to this repo.
  • Can be tested manually via workflow_dispatch in the Actions tab once the credentials are live. The manual trigger will pop up a text box asking which model you want to update.
  • Standard automated runs will now trigger seamlessly behind the scenes once the upstream build workflow succeeds.

@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.07%. Comparing base (39285e4) to head (d402daa).
⚠️ Report is 2 commits behind head on develop.

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           
Flag Coverage Δ
fair 96.06% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dakotabenjamin dakotabenjamin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

After clarity from discussion hotosm/k8s-infra#183 this is good to go. App was added and branch protection configured.

Comment thread .github/workflows/deploy-knative.yml Outdated
# 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"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This would need to get updated as new models approved?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like it. An env var at the top of the file would be much more logical than buried here

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes should be dynamic !

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Comment thread .github/workflows/deploy-knative.yml Outdated
sudo chmod a+x /usr/local/bin/yq

- name: Login to GHCR
uses: docker/login-action@v3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@v4

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

on manual trigger , we can get teh manual input for the model_folder name and all this scanning is not needed !

@yshmael yshmael Aug 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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!

Comment thread .github/workflows/deploy-knative.yml Outdated
# 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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@spwoodcock spwoodcock Aug 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good call, we want the sha.

The latest gh-workflows do this 👍

@spwoodcock

spwoodcock commented Aug 5, 2026

Copy link
Copy Markdown
Member

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:

  1. Have we considered having this raising a PR instead, instead of pushing directly? That would at least add another layer of human control, to avoid the worst scenario

  2. What other approaches did you consider before settling on this one? Pros / cons?

@yshmael

yshmael commented Aug 5, 2026

Copy link
Copy Markdown
Author

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?
That is a great point, and definitely the safest route. My initial thought was to use direct pushes to keep deployment velocity high for our lower environments (develop and staging). However, we can absolutely switch this workflow to open a PR instead of a direct commit. A good middle ground might be allowing direct pushes to the develop/staging branches, but strictly generating PRs for main. I am happy to pivot to a PR-generation model if you prefer that level of control!

2. What other approaches did we consider?

  • ArgoCD Image Updater: I know we use this pattern elsewhere in the cluster (like the kube-ops-view app).
    • Pros: Native to Argo, no cross-repo GitHub tokens needed.
    • Cons: It relies on polling the registry. We wanted an explicit, event-driven link where a successful model build immediately and synchronously updates the environment.
  • Combined CI/CD Script: Initially, I had the Docker build and GitOps updates in the same script.
    • Pros: Simple to write.
    • Cons: Duplicated build steps and mixed responsibilities. I actually just refactored this based on Kshitij's feedback. This script is now purely a CD orchestrator that only triggers via workflow_run after the models are successfully built in the main pipeline.

Let me know how you'd like to proceed! I can swap the git push command for a PR generator if you want to lock down the direct commits.

@spwoodcock

Copy link
Copy Markdown
Member

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?

@yshmael

yshmael commented Aug 5, 2026

Copy link
Copy Markdown
Author

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!

@yshmael

yshmael commented Aug 5, 2026

Copy link
Copy Markdown
Author

@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.

@spwoodcock

Copy link
Copy Markdown
Member

Can we use the reusable workflows from hotosm/gh-workflows? The latest v4.x.x workflows include a sha in the tags

@dakotabenjamin
dakotabenjamin self-requested a review August 5, 2026 13:18
@yshmael

yshmael commented Aug 7, 2026

Copy link
Copy Markdown
Author

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!

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.

4 participants