Repository navigation
chore(ci): Deploy preview to github - #7353
PeterSchafer wants to merge 1 commit into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
This comment has been minimized.
This comment has been minimized.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Addressed the Bugbot finding in f6c4c86.
Fixed by scoping both to |
|
@PeterSchafer sign the commit please 😃 |
This comment has been minimized.
This comment has been minimized.
f6c4c86 to
5f8277a
Compare
This comment has been minimized.
This comment has been minimized.
|
Fixed the "dirty tag" issue flagged by Bugbot and the PR Reviewer Guide on The previous Switched to the suggested approach: git tag --list 'v[0-9]*.[0-9]*.[0-9]*' --sort=-version:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$' | head -n 1This resolves the highest stable tag by semver directly, regardless of which commit it's reachable from — which also fixes the case where the latest stable tag only exists on an unmerged Verified in a scratch repo with preview tags on top of a stable tag; output is now a clean On the "sign the commit" request — I don't have a GPG/SSH signing key configured in this sandbox, so I can't sign commits from here. @PeterSchafer, you may want to sign this commit locally (or re-sign the branch) once you pull it down. 🤖 Generated with Claude Code |
|
Fixed the "dirty tag" issue flagged by Bugbot and the PR Reviewer Guide on The previous Switched to the suggested approach: git tag --list 'v[0-9]*.[0-9]*.[0-9]*' --sort=-version:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$' | head -n 1This resolves the highest stable tag by semver directly, regardless of which commit it's reachable from — which also fixes the case where the latest stable tag only exists on an unmerged release/* branch. Verified in a scratch repo with preview tags on top of a stable tag; output is now a clean vX.Y.Z in both cases. On the "sign the commit" request — I don't have a GPG/SSH signing key configured in this sandbox, so I can't sign commits from here. @PeterSchafer, you may want to sign this commit locally (or re-sign the branch) once you pull it down. 🤖 Generated with Claude Code |
eb1e9f3 to
d78a826
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Fixed the CI failure ( Root cause: git tag --list '...' --sort=-version:refname | grep -E '...' | head -n 1With enough tags, Fix: capture the full filtered tag list into a variable first, then take the first line in bash ( Verified locally by stress-testing the old pipeline against a repo with 20k tags (reproduced exit 141 intermittently) and confirming the new version returns the correct tag consistently across 50 runs, plus the empty-tags edge case. 🤖 Generated with Claude Code |
abf1b4f to
a8e4efd
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
a8e4efd to
736eb7d
Compare
This comment has been minimized.
This comment has been minimized.
736eb7d to
507386d
Compare
This comment has been minimized.
This comment has been minimized.
507386d to
c117d09
Compare
This comment has been minimized.
This comment has been minimized.
c117d09 to
714247a
Compare
This comment has been minimized.
This comment has been minimized.
714247a to
c5e4361
Compare
This comment has been minimized.
This comment has been minimized.
c5e4361 to
34f7afe
Compare
This comment has been minimized.
This comment has been minimized.
| echo "Checking for new versions..." | ||
|
|
||
| latest_version_full=$(git describe --tags `git rev-list --tags --max-count=1`) | ||
| latest_version_full=$(./release-scripts/latest-stable-tag.sh) |
There was a problem hiding this comment.
In Makefile line 109, it builds the RELEASE_NOTES.md with:
$(BINARY_OUTPUT_FOLDER)/RELEASE_NOTES.md:
npx conventional-changelog-cli -l -r 1 -n ./release-scripts/conventional-changelog-cli-config.js > $(BINARY_OUTPUT_FOLDER)/RELEASE_NOTES.mdI believe it is something to take a look too.
| # Prints the highest stable release tag (vX.Y.Z, no pre-release suffix) by | ||
| # semver, ignoring preview/rc tags and independent of which commit they are | ||
| # reachable from (so this also works for a stable tag that only exists on an | ||
| # unmerged release/* branch). | ||
| # | ||
| # Deliberately not piped through `head -n 1`: with enough tags, head can | ||
| # close the pipe before git/grep finish writing, and under `pipefail` the | ||
| # resulting SIGPIPE surfaces as a build failure even though the correct tag | ||
| # was already produced. | ||
|
|
||
| stable_tags="$(git tag --list 'v[0-9]*.[0-9]*.[0-9]*' --sort=-version:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$' || true)" | ||
|
|
||
| if [ -z "$stable_tags" ]; then | ||
| echo "No stable release tags found" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "${stable_tags%%$'\n'*}" |
There was a problem hiding this comment.
does these "stable_tags" also includes the hotfix releases?
Wondering about this name "stable_tags", if should be something else
| // TestSkipDeploymentRunsOnlyForMatchingChannel is a regression test for the | ||
| // deployment gate: a CircleCI job must run (skip=false) only when the | ||
| // requested channel matches the branch's actual, git-derived release | ||
| // channel, and must be skipped (skip=true) for every other channel. This is | ||
| // the only thing standing between the CircleCI `deployment` parameter and | ||
| // the branch-derived RELEASE_CHANNEL disagreeing with each other. | ||
| func TestSkipDeploymentRunsOnlyForMatchingChannel(t *testing.T) { |
There was a problem hiding this comment.
please remove these comments and others that have been added in this PR. I know LLM adds a lot of these comments, but the code should be readable by itself. These comments are often confusing or can cause maintenance burden in the future.
There was a problem hiding this comment.
The changes in this file seems to be unrelated to this PR?
There was a problem hiding this comment.
The tests now run as a part of the pipeline and were failing.
| # the "latest release" pointer consumed by homebrew-tap, scoop-snyk, and install | ||
| # scripts keeps resolving to stable only. | ||
| local prerelease_arg="" | ||
| if [ -n "${RELEASE_CHANNEL}" ] && [ "${RELEASE_CHANNEL}" != "stable" ]; then |
There was a problem hiding this comment.
this check is being performed in two places (line 119).
Suggestion: Extract a small is_stable_release helper.
| # Prints the highest stable release tag (vX.Y.Z, no pre-release suffix) by | ||
| # semver, ignoring preview/rc tags and independent of which commit they are | ||
| # reachable from (so this also works for a stable tag that only exists on an | ||
| # unmerged release/* branch). | ||
| # | ||
| # Deliberately not piped through `head -n 1`: with enough tags, head can | ||
| # close the pipe before git/grep finish writing, and under `pipefail` the | ||
| # resulting SIGPIPE surfaces as a build failure even though the correct tag | ||
| # was already produced. | ||
|
|
||
| stable_tags="$(git tag --list 'v[0-9]*.[0-9]*.[0-9]*' --sort=-version:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$' || true)" |
There was a problem hiding this comment.
I guess these comments can be shortened. Possibly the second part of it can be removed, WDYT?
| echo "Checking for new versions..." | ||
|
|
||
| latest_version_full=$(git describe --tags `git rev-list --tags --max-count=1`) | ||
| latest_version_full=$(./release-scripts/latest-stable-tag.sh) |
There was a problem hiding this comment.
Similar to next-version.sh, should it make use of $(dirname "$0")/latest-stable-tag.sh instead of the relative path here?
| func TestLatestStableTagIgnoresMoreRecentPreviewTags(t *testing.T) { | ||
| dir := newTagTestRepo(t) | ||
| tagCommit(t, dir, "v1.0.0") | ||
| newCommit(t, dir, "preview 1") | ||
| tagCommit(t, dir, "v1.1.0-preview.deadbeef") | ||
| newCommit(t, dir, "preview 2") | ||
| tagCommit(t, dir, "v1.1.0-preview.cafebabe") | ||
|
|
||
| got, err := runLatestStableTag(t, dir) | ||
| if err != nil { | ||
| t.Fatalf("expected success, got error: %v\noutput: %s", err, got) | ||
| } | ||
| if got != "v1.0.0" { | ||
| t.Fatalf("expected clean stable tag v1.0.0, got %q", got) | ||
| } | ||
| } | ||
|
|
||
| func TestLatestStableTagPicksHighestSemverNotMostRecentlyCreated(t *testing.T) { | ||
| dir := newTagTestRepo(t) | ||
| // Tags created out of semver order, mirroring a patch release cut from an | ||
| // older branch after a newer minor was already tagged. | ||
| tagCommit(t, dir, "v1.2.0") | ||
| newCommit(t, dir, "c2") | ||
| tagCommit(t, dir, "v1.1.5") | ||
|
|
||
| got, err := runLatestStableTag(t, dir) | ||
| if err != nil { | ||
| t.Fatalf("expected success, got error: %v\noutput: %s", err, got) | ||
| } | ||
| if got != "v1.2.0" { | ||
| t.Fatalf("expected highest stable tag v1.2.0, got %q", got) | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: some tests can be converted to a table-testing format
| // TestLatestStableTagSurvivesLargeTagCountUnderPipefail is a regression test | ||
| // for a SIGPIPE race: piping the tag list through `head -n 1` let head close | ||
| // the pipe before git/grep finished writing once there were enough tags, | ||
| // which `set -o pipefail` turned into an intermittent build failure (exit | ||
| // 141) even though the correct tag had already been produced. | ||
| func TestLatestStableTagSurvivesLargeTagCountUnderPipefail(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("skipping large tag count test in short mode") | ||
| } | ||
|
|
||
| dir := newTagTestRepo(t) | ||
| const tagCount = 20000 | ||
|
|
||
| // git tag only accepts one tag per invocation; batch creation via | ||
| // update-ref is far faster than tagCount separate `git tag` calls. | ||
| var stdin bytes.Buffer | ||
| for i := 1; i <= tagCount; i++ { | ||
| stdin.WriteString("create refs/tags/v1.0." + strconv.Itoa(i) + " HEAD\n") | ||
| } |
There was a problem hiding this comment.
Do we need this test? It creates 20k tags and runs the script 20 times on every CI run, which makes the test suite slower. The SIGPIPE race it covers is timing-dependent, so it could still pass if the bug came back. I'd lower the numbers or remove it.
| - test-release-scripts: | ||
| context: | ||
| - devex_cli_docker_hub | ||
| requires: | ||
| - prepare-build | ||
| filters: | ||
| branches: | ||
| ignore: | ||
| - '/^docs\/.*/' |
There was a problem hiding this comment.
test-release-scripts isn't in any release job's requires, so a failure doesn't block deploys. Is this intended?
There was a problem hiding this comment.
release jobs run on release branches, tests run on non release branches. a change to the scripts needs to go through a non release branch and thereby tests are executed. This is true for all tests.
34f7afe to
a996329
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a996329. Configure here.
| @@ -97,6 +106,7 @@ upload_github() { | |||
| else | |||
| echo "Uploading to GitHub..." | |||
There was a problem hiding this comment.
Should we update the logic to check if the release exists before attempting to create or upload?
Perhaps:
if gh release view "${VERSION_TAG}" >/dev/null 2>&1; then
echo "Release ${VERSION_TAG} already exists, uploading assets..."
gh release upload "${VERSION_TAG}" "${StaticFiles[@]}" --clobber
else
echo "Uploading to GitHub..."
gh release create "${VERSION_TAG}" "${StaticFiles[@]}" \
${prerelease_arg} \
--target "${CIRCLE_SHA1}" \
--title "${VERSION_TAG}" \
--notes-file binary-releases/RELEASE_NOTES.md
fi
GitHub API hiccups can occasionally cause a release creation step to fail midway or fail to register as complete. Currently, if this step fails, re-running the job fails because the release tag already exists, requiring someone to manually delete the release before re-triggering. This has happened on other releases 😅
There was a problem hiding this comment.
I would say that this would increase the scope of the task too much.
a996329 to
2d5a9e9
Compare
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2d5a9e9 to
a966fd7
Compare
PR Reviewer Guide 🔍
|
4 participants
Pull Request Submission Checklist
are release-note ready, emphasizing
what was changed, not how.
What does this PR do?
Additionally deploy preview CLI to github. Since this will create preview tags, they need to be ignored in different release scripts. This is why many scripts are touched as well. I used the chance to ad some tests for pipeline scripts as well so that their behaviour can better be validated.
Where should the reviewer start?
How should this be manually tested?
What's the product update that needs to be communicated to CLI users?
Note
Medium Risk
Changes release pipeline and GitHub/npm “latest” semantics; mistakes could publish wrong channel or block legitimate stable releases, but behavior is covered by new release-script tests.
Overview
Adds preview CLI publishing to GitHub from
main(new CircleCIupload github (preview)after S3 preview) and makesrelease-githubdeployment-aware so Jira release creation runs only for stable.Preview/rc GitHub uploads are marked
--prereleaseso GitHub “latest” stays on stable for Homebrew, Scoop, and install flows. Versioning and pre-release checks now uselatest-stable-tag.sh(highestvX.Y.Z, ignoring preview tags) innext-version.shandcreate-release.sh, withvalidate-repository.shtightened to tags on HEAD and exact stable tag collisions only.RELEASE_NOTES.mdgeneration passes--skip-unstableto conventional-changelog. CI addstest-release-scripts(make test-release-scripts) and Go tests cover channel mapping, skip-deployment, latest-stable-tag, next-version, and validate-repository behavior.Reviewed by Cursor Bugbot for commit a966fd7. Bugbot is set up for automated code reviews on this repo. Configure here.