Skip to content

fix: Github checkout#5972

Open
wayfarer3130 wants to merge 4 commits into
masterfrom
fix/github-publish
Open

fix: Github checkout#5972
wayfarer3130 wants to merge 4 commits into
masterfrom
fix/github-publish

Conversation

@wayfarer3130
Copy link
Copy Markdown
Contributor

@wayfarer3130 wayfarer3130 commented Apr 21, 2026

Context

Github releases are not properly supported by lerna any longer
No functional change

Changes & Results

Testing

Checklist

PR

  • [] My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • [] My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • [] The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • [] OS:
  • [] Node version:
  • [] Browser:

Greptile Summary

This PR replaces lerna's broken GitHub release support with a direct curl call to the GitHub Releases API in the CircleCI publish job, and removes --create-release github from the lerna version invocation in publish-version.mjs. The shell script is well-guarded (checks for GH_TOKEN and version.txt) and the tag is pushed by publish-version.mjs before this step runs, so ordering is correct.

Confidence Score: 5/5

Safe to merge — the change is a straightforward replacement of broken lerna release support with a direct GitHub API call, with no functional regressions.

Only P2 findings present (redundant body field with generate_release_notes). The core logic is correct: tag is pushed before the release step, guards for missing env vars and version.txt are in place, and error cases exit non-zero.

No files require special attention.

Important Files Changed

Filename Overview
.circleci/config.yml Adds a new CI step that creates a GitHub release via the API directly, replacing lerna's broken release support; handles 422 as "already exists" which was flagged in a previous thread
publish-version.mjs Removes --create-release github from the lerna version command since lerna no longer properly supports GitHub releases; straightforward cleanup
Prompt To Fix All With AI
This is a comment left during a code review.
Path: .circleci/config.yml
Line: 207-210

Comment:
**Redundant `body` when `generate_release_notes` is enabled**

When both `body` and `generate_release_notes: true` are sent, GitHub prepends the static `body` string to the auto-generated changelog rather than replacing it. The result is `"Automated release for vX.Y.Z\n\n<generated notes>"`. If the intent is a fully auto-generated body, drop the `body` field; if a custom header is desired, the current behavior is fine — just worth being intentional about it.

```suggestion
"draft": false,
              "prerelease": ${IS_PRERELEASE},
              "generate_release_notes": true
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/mas..." | Re-trigger Greptile

@wayfarer3130 wayfarer3130 requested a review from sedghi April 21, 2026 14:19
@netlify
Copy link
Copy Markdown

netlify Bot commented Apr 21, 2026

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit a88fcbd
🔍 Latest deploy log https://app.netlify.com/projects/ohif-dev/deploys/69ef673598e13e0008f42508
😎 Deploy Preview https://deploy-preview-5972--ohif-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Comment thread .circleci/config.yml
Comment on lines +225 to +226
elif [ "$HTTP_CODE" = "422" ]; then
echo "Release for ${TAG} already exists; treating as success."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Overly broad 422 suppression

A 422 from the GitHub Releases API means any validation failure (bad JSON, invalid tag format, duplicate name, etc.), not just an already-existing release. Silently treating all 422 responses as success will hide misconfiguration bugs. To be safe, parse the response and check for the specific "already_exists" error code before suppressing the failure.

Prompt To Fix With AI
This is a comment left during a code review.
Path: .circleci/config.yml
Line: 225-226

Comment:
**Overly broad 422 suppression**

A 422 from the GitHub Releases API means *any* validation failure (bad JSON, invalid tag format, duplicate name, etc.), not just an already-existing release. Silently treating all 422 responses as success will hide misconfiguration bugs. To be safe, parse the response and check for the specific `"already_exists"` error code before suppressing the failure.

How can I resolve this? If you propose a fix, please make it concise.

Comment thread publish-version.mjs
'--message',
`chore(version): Update package versions to ${nextVersion} [skip ci]`,
'--conventional-commits',
'--create-release',
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.

so why is this not working anymore?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated lerna version

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Might also be missing GH_TOKEN, which is a silent failure in newer lerna versions.

@cypress
Copy link
Copy Markdown

cypress Bot commented Apr 21, 2026

Viewers    Run #6208

Run Properties:  status check passed Passed #6208  •  git commit a88fcbdc78: Merge remote-tracking branch 'origin/master' into fix/github-publish
Project Viewers
Branch Review fix/github-publish
Run status status check passed Passed #6208
Run duration 02m 15s
Commit git commit a88fcbdc78: Merge remote-tracking branch 'origin/master' into fix/github-publish
Committer Bill Wallace
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 37
View all changes introduced in this branch ↗︎

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.

2 participants