Skip to content

fix: gate wave release on master and release-* branches DEVOPS-1531#1072

Open
ShahzebMahmood wants to merge 5 commits into
masterfrom
fix/DEVOPS-1531-wave-enterprise-push-master-only
Open

fix: gate wave release on master and release-* branches DEVOPS-1531#1072
ShahzebMahmood wants to merge 5 commits into
masterfrom
fix/DEVOPS-1531-wave-enterprise-push-master-only

Conversation

@ShahzebMahmood

@ShahzebMahmood ShahzebMahmood commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

A [release] commit now only triggers the release steps when pushed from master or a release-* branch. On any other branch it logs a warning and skips, instead of failing at the wave-ecr-pusher OIDC assume.

Run 28649252557 failed with Not authorized to perform sts:AssumeRoleWithWebIdentity because a [release] Bump 1.35.1-B1 commit was pushed from the feature branch add-which-tool-conda-templates. The wave-ecr-pusher trust only allows master and release-* (which is correct), so the assume was rightly rejected. The real issue is that detect-release fired a full release from any branch with [release] in the commit.

This aligns the workflow trigger with the existing OIDC trust. No infra change needed.

DEVOPS-1531

@pditommaso

Copy link
Copy Markdown
Collaborator

Is this really needed? we need to test features with temporary releases, and this constraint adds a lot of friction

@ShahzebMahmood ShahzebMahmood requested a review from bebosudo July 7, 2026 12:08
@gavinelder

Copy link
Copy Markdown
Contributor

The intent is to prevent internal releases and beta releases surfacing in customer deployed locations and limit their use in production environment.

There should still be a retained capability to cut a container from a branch for use in development environments for testing and validation.

@ShahzebMahmood can you look at this PR with that context.

We are trying to ensure the public release of wave artifacts to enterprise customers and consumers meet our quality bar.

We are not trying to require a merge before container build for testing and development, for example pr-preview image creation.

having a tag such as [preview] which mirrors [release] which pushes to dev could be a potential approach.

@ShahzebMahmood

Copy link
Copy Markdown
Contributor Author

The intent is to prevent internal releases and beta releases surfacing in customer deployed locations and limit their use in production environment.

There should still be a retained capability to cut a container from a branch for use in development environments for testing and validation.

@ShahzebMahmood can you look at this PR with that context.

We are trying to ensure the public release of wave artifacts to enterprise customers and consumers meet our quality bar.

We are not trying to require a merge before container build for testing and development, for example pr-preview image creation.

having a tag such as [preview] which mirrors [release] which pushes to dev could be a potential approach.

Yes, working on this

@ShahzebMahmood ShahzebMahmood self-assigned this Jul 8, 2026
@ShahzebMahmood

Copy link
Copy Markdown
Contributor Author

Reworked this based on your feedback. Two tags now:

[preview] builds from any branch and pushes to the internal dev ECR only, tagged with the commit sha so previews don't overwrite each other. No merge needed, so it covers the pr-preview and testing case.

[release] is the customer path. Only on master or a release-* branch does it push to the enterprise repo, publish maven, and cut the git tag and release. Anything else just warns and pushes nothing.

Let me know if that matches what you had in mind.

@ShahzebMahmood ShahzebMahmood marked this pull request as ready for review July 8, 2026 11:26
@ShahzebMahmood ShahzebMahmood requested a review from gavinelder July 8, 2026 11:26
@ShahzebMahmood

Copy link
Copy Markdown
Contributor Author

@claude

@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown

Claude encountered an error —— View job


I'll analyze this and get back to you.

1 similar comment
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown

Claude encountered an error —— View job


I'll analyze this and get back to you.

@ShahzebMahmood

Copy link
Copy Markdown
Contributor Author

@claude

@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

Claude finished @ShahzebMahmood's task in 1m 23s —— View job


PR Review

  • Gather context
  • Read changed workflow file
  • Analyze changes and provide review

Summary

The implementation looks correct and well-structured. The two-tier approach ([preview] / [release]) directly addresses the concerns raised in the discussion. Below are my observations.

Correctness

Branch glob matching (line 77) — Inside [[ ]], the unquoted refs/heads/release-* is treated as a glob pattern, so it correctly matches refs/heads/release-1.35, etc. This is intentional and correct bash behavior.

has_tag + && chaining (lines 67–68) — Works correctly because the variables are initialized to false beforehand and the && short-circuits on non-zero exit. No set -e concern since neither the detect-release step nor its sub-steps use set -e.

is_release output removed — The old downstream conditions steps.detect-release.outputs.is_release == 'true' are all updated to is_publish. No dangling references.

github.event.head_commit.message on PRs — For pull_request events this field is null, so COMMIT_MSG is empty and all tags evaluate to false. This is the correct behavior — PR builds never trigger image builds.

Minor Observations

  1. [preview] on master without [release] — A commit tagged only [preview] on master will build and push to the internal ECR but not the enterprise repo. This may be surprising to someone who expects [preview] to be a feature-branch-only concept. Worth documenting in the project's contributing guide.

  2. Both tags in one commit — A commit like [preview][release] Bump 1.35.1 on master sets both is_preview=true and is_publish=true, causing a push to both internal ECR and the enterprise repo. The image is still only built once (via the shared build step), which is correct and efficient.

  3. Preview image tag format — The ${GITHUB_SHA::7} substring in line 179 assumes bash 4+. GitHub-hosted ubuntu-latest runners ship with bash 5, so this is fine — just worth noting if ever running locally on macOS default shell.

  4. set -u missing from release step — The Promote release step uses set -e and set -x but not set -u or set -o pipefail. The other steps use set -euo pipefail. Minor inconsistency, low risk given the simple variable usage there.

  5. role-session-name for internal pusher — Set to ${{ github.event.repository.name }} (e.g., wave) rather than the run-ID-based name used elsewhere. The comment explains this carries the repository into the session tag for the role trust — just make sure that aligns with the gha-generic-internal-pusher trust policy as documented.

No blocking issues

The logic correctly prevents [release] from reaching OIDC roles that only trust master/release-*, and the [preview] path gives developers a low-friction way to cut a dev image from any branch. The single Build wave server image step avoiding double Gradle/JIB builds is a nice touch.

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.

3 participants