fix: do not update manifest on PRs without stack label #331

wontonst · 2025-01-15T18:03:06Z

Attempts to address https://github.com/chanzuckerberg/argus/issues/655 by running Update ArgoCD manifests only if a PR associated with a push has the stack label.

Tested via https://github.com/chanzuckerberg/core-platform-example-app/pull/112 which is on 2.X of this action (hence the merge conflicts).

If we like this approach, what should I do? Maybe create 2.16.0 and 3.7.0? Do we care about supporting 2.X apps? It would be simpler to ask those app owners to use 3.X to gain this benefit. Prior to merging I will rebase on latest and test again on an app using 3.X.

Requires application owners to upgrade their argus-docker-build.yaml version target

after merging update the tag of build-prep

wontonst · 2025-01-15T18:04:58Z

.github/workflows/argus-docker-build.yaml

+        with:
+          filterOutClosed: true
+          filterOutDraft: false
+      - uses: chanzuckerberg/github-actions/.github/actions/argus-builder/build-prep@rzheng/stack-label


what is our versioning strategy? continue using a sha1? leave it pointing to a branch like this PR? maybe we can create a tag?

we use release-please to propose releases based on our conventional commits and merging the release proposal PR creates the actual release/tag. the reason this uses a sha instead of one of those tags is that it allows us to make a single release with the update instead of first releasing build-prep and then coming back and updating the version of build-prep used in this file and making another release

wontonst · 2025-01-15T18:17:58Z

.github/actions/argus-builder/build-prep/action.yml

@@ -120,4 +134,5 @@ runs:
        script: |
          const branchMatched = `${{ steps.branch_filter.outputs.match }}` === 'true';
          const filesMatched = `${{ steps.file_filter.outputs.run_on }}` === 'true';
-          core.setOutput('should_build', filesMatched && branchMatched);
+          const prHasStackLabel = `${{ steps.require_stack_label.outputs.has_label }}` === 'true';
+          core.setOutput('should_build', filesMatched && branchMatched && prHasStackLabel);


note this also skips Update Manifest in manifest-update.

is that okay or should I modify the manifest-update action to only skip the step that adds the values.yaml change?

oh wait that step only writes the values.yml changes so it's safe to skip

.github/workflows/argus-docker-build.yaml

jakeyheath · 2025-01-16T19:15:13Z

.github/actions/argus-builder/build-prep/action.yml

+  labels:
+    description: 'Labels attached to an associated PR'
+    requred: false
+    default: ''


rename to something that indicates these are the labels currently attached to the PR. Then we need another input for the name of the label with the default value being "stack". I think we should allow for the name of the label to be configured, with a sane default

jakeyheath · 2025-01-16T19:15:47Z

.github/actions/argus-builder/build-prep/action.yml

+      with:
+        script: |
+          const labels = `${{ inputs.labels }}`;
+          let hasStackLabel = labels.split(',').includes("stack")


Suggested change

let hasStackLabel = labels.split(',').includes("stack")

let hasStackLabel = labels.split(',').includes("{{inputs.label}}")

see comment above

wontonst · 2025-01-17T18:10:21Z

PR should be ready for review now.

Rebased this PR latest 3.X
Upgraded core sample app to 3.6.0
Tested this change in https://github.com/chanzuckerberg/core-platform-example-app/pull/112/files

Todo:

Pin argus-builder/build-prep to a commit that is approved by reviewers
Bump argus-docker-build version in relevant repos

Side note I noticed that rerunning jobs via the Github UI when a values.yaml commit has been created always results in the manifest update step not getting executed. My guess is because the file filters thinks the base commit is the commit before the values.yaml file commit so it thinks there are no files matching the path filters. I think we can ignore this since most folks are probably pushing to their branch to trigger a build but good to know since I wasted time figuring out why steps were getting skipped.

.github/actions/argus-builder/build-prep/action.yml

hspitzley-czi · 2025-01-17T21:35:01Z

.github/actions/argus-builder/build-prep/action.yml

-          core.setOutput('should_build', filesMatched && branchMatched);
+          const prHasStackLabel = `${{ steps.require_stack_label.outputs.has_trigger_label }}` === 'true';
+          const shouldBuild = filesMatched && branchMatched && prHasStackLabel;
+          core.setOutput('should_build', shouldBuild);


it might be good to have the image still get built, the benefits of that would be even without making a stack a user would still be able to see whether their code introduced a bug that prevents the image from building. @jakeyheath what do you think?

I think given that the main artifact for a build is often times an image, and given the low cost of building an image, we should just always build an image. what do we think about removing should_build from build-docker's if condition?

if: needs.prep.outputs.should_build == 'true' && needs.prep.outputs.images != '[]'

alternatively I can create a new output like shouldDeploy and use that to separately control Update manifest

yea that would be more clear @wontonst . Let's go forward with that

hspitzley-czi · 2025-01-21T20:43:34Z

.github/actions/argus-builder/build-prep/action.yml

+    description: 'Labels attached to an associated PR'
+    required: false
+    default: ''
+  manifest_trigger_label:


I think this needs to allow multiple values, comma delimited

We don't have anyone with this setup right now but we've previously had people build a monorepo with multiple Argus apps that use independent labels to control stack creation. We have an example monorepo set up to work like that: https://github.com/chanzuckerberg/argus-example-monorepo

hmm, this could get confusing. some folks might want it to mean "any of these labels should trigger a manifest update" and others "these labels must all be applied to trigger a manifest update"

what do you think about crossing that bridge when we come to it?

I'm good with that, alternatively it could take a regex which would make it support the "any of these labels" case. Up to you

jakeyheath · 2025-01-23T19:43:00Z

.github/workflows/argus-docker-build.yaml

+        with:
+          filterOutClosed: true
+          filterOutDraft: false
+      - uses: chanzuckerberg/github-actions/.github/actions/argus-builder/build-prep@rzheng/stack-label


Don't forget to change this in a follow up PR

wontonst · 2025-01-24T19:41:25Z

tested again at https://github.com/chanzuckerberg/core-platform-example-app/actions/runs/12956338859/job/36142386737?pr=112 https://github.com/chanzuckerberg/core-platform-example-app/actions/runs/12956387261/job/36142533059?pr=112

prep now outputs should_deploy which controls Update Manifest

wontonst · 2025-01-24T21:48:39Z

will defer upgrading other repos until https://czi.atlassian.net/browse/CCIE-3947

wontonst commented Jan 15, 2025

View reviewed changes

hspitzley-czi reviewed Jan 15, 2025

View reviewed changes

.github/workflows/argus-docker-build.yaml Show resolved Hide resolved

jakeyheath reviewed Jan 16, 2025

View reviewed changes

wontonst force-pushed the rzheng/stack-label branch 8 times, most recently from b7a9dca to 280fc38 Compare January 17, 2025 17:50

wontonst marked this pull request as ready for review January 17, 2025 18:10

hspitzley-czi reviewed Jan 17, 2025

View reviewed changes

.github/actions/argus-builder/build-prep/action.yml Outdated Show resolved Hide resolved

hspitzley-czi reviewed Jan 17, 2025

View reviewed changes

wontonst force-pushed the rzheng/stack-label branch from 280fc38 to 37be64d Compare January 21, 2025 18:33

hspitzley-czi reviewed Jan 21, 2025

View reviewed changes

wontonst mentioned this pull request Jan 23, 2025

feat!: CCIE-3896 rename CZI_GITHUB_HELPER_* to GH_ACTIONS_HELPER_* #333

Merged

jakeyheath reviewed Jan 23, 2025

View reviewed changes

jakeyheath approved these changes Jan 23, 2025

View reviewed changes

wontonst force-pushed the rzheng/stack-label branch 2 times, most recently from a0a27c4 to 23a4c16 Compare January 24, 2025 19:20

fix: CCIE-3899 do not update manifest on PRs without stack label

9721128

wontonst force-pushed the rzheng/stack-label branch from 23a4c16 to 9721128 Compare January 24, 2025 19:33

wontonst merged commit 90e3b17 into main Jan 24, 2025
9 checks passed

wontonst deleted the rzheng/stack-label branch January 24, 2025 19:42

czi-github-helper bot mentioned this pull request Jan 24, 2025

chore: release 3.6.1 #334

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: do not update manifest on PRs without stack label #331

fix: do not update manifest on PRs without stack label #331

wontonst commented Jan 15, 2025 •

edited

Loading

wontonst Jan 15, 2025

hspitzley-czi Jan 15, 2025

wontonst Jan 15, 2025 •

edited

Loading

wontonst Jan 17, 2025

jakeyheath Jan 16, 2025

wontonst Jan 16, 2025

jakeyheath Jan 16, 2025

wontonst Jan 16, 2025

wontonst commented Jan 17, 2025 •

edited

Loading

hspitzley-czi Jan 17, 2025

wontonst Jan 21, 2025

jakeyheath Jan 22, 2025

hspitzley-czi Jan 21, 2025

wontonst Jan 21, 2025

hspitzley-czi Jan 21, 2025 •

edited

Loading

jakeyheath Jan 23, 2025

wontonst Jan 24, 2025

wontonst commented Jan 24, 2025

wontonst commented Jan 24, 2025

	let hasStackLabel = labels.split(',').includes("stack")
	let hasStackLabel = labels.split(',').includes("{{inputs.label}}")

fix: do not update manifest on PRs without stack label #331

fix: do not update manifest on PRs without stack label #331

Conversation

wontonst commented Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wontonst Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wontonst commented Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hspitzley-czi Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wontonst commented Jan 24, 2025

wontonst commented Jan 24, 2025

wontonst commented Jan 15, 2025 •

edited

Loading

wontonst Jan 15, 2025 •

edited

Loading

wontonst commented Jan 17, 2025 •

edited

Loading

hspitzley-czi Jan 21, 2025 •

edited

Loading