chore: upload posthog-js dist to S3 on release#3307
Open
dustinbyrne wants to merge 9 commits intomainfrom
Open
chore: upload posthog-js dist to S3 on release#3307dustinbyrne wants to merge 9 commits intomainfrom
dustinbyrne wants to merge 9 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 No Changeset FoundThis PR doesn't include a changeset. A changeset (and the release label) is required to release a new version. How to add a changesetRun this command and follow the prompts: pnpm changesetRemember: Never use |
Contributor
Contributor
|
Size Change: 0 B Total Size: 6.65 MB ℹ️ View Unchanged
|
dustinbyrne
force-pushed
the
chore/s3-upload-release
branch
from
ddb4666 to
b064f9b
Compare
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: .github/workflows/release.yml
Line: 437
Comment:
**`notify-released` silently suppressed for non-posthog-js releases**
When `upload-s3` is **skipped** (i.e. the release doesn't include a posthog-js version bump), `needs.upload-s3.result` equals `'skipped'`, not `'success'`. The new condition therefore evaluates to `false`, and the "All packages released successfully!" Slack notification is never sent — even for a fully successful release of e.g. `posthog-node` or `@posthog/ai`.
This is a silent regression that will affect every release that doesn't bump posthog-js.
Fix: treat `skipped` as an acceptable outcome alongside `success`:
```suggestion
if: always() && needs.publish.result == 'success' && (needs.upload-s3.result == 'success' || needs.upload-s3.result == 'skipped') && needs.notify-approval-needed.outputs.slack_ts != ''
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: .github/scripts/upload-posthog-js-s3.sh
Line: 39
Comment:
**`TMPDIR` shadows a POSIX reserved environment variable**
`TMPDIR` is a POSIX-reserved environment variable that many tools — including `mktemp` itself on some platforms — read to locate the system temp directory. Overwriting it means any subsequent `mktemp` call in this script (or a sourced helper) would try to nest new temps inside the directory you just created, and other tools invoked as subprocesses may also be affected.
Use a non-reserved name instead:
```suggestion
TMPWORKDIR="$(mktemp -d)"
trap 'rm -rf "$TMPWORKDIR"' EXIT
```
And update the references below (`$TMPDIR/versions.json` → `$TMPWORKDIR/versions.json`, etc.).
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "ci: split build and upload into distinct..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
marandaneto
reviewed
Apr 1, 2026
marandaneto
approved these changes
Apr 1, 2026
Piccirello
approved these changes
Apr 1, 2026
Comment on lines
+57
to
+60
| jq --arg v "$VERSION" --arg ts "$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ | ||
| '. + [{"version": $v, "timestamp": $ts}]' "$TMPDIR/versions.json" > "$TMPDIR/versions_updated.json" | ||
| aws s3 cp "$TMPDIR/versions_updated.json" "s3://$BUCKET/versions.json" \ | ||
| --content-type "application/json" |
Member
There was a problem hiding this comment.
This seems like the riskiest bit to me, since one bad change will break the entire file. I'm assuming using jq to modify the file means it will only ever generate valid json. Is there any additional check we can run here to verify the contents match what we expect before publishing?
Contributor
Author
There was a problem hiding this comment.
I've added some additional verification's using jq:
- It's a JSON array
- The length of the array is exactly original + 1
- Every entry is of the expected schema
This is a good point though. Breaking this file would mean breaking version synchronization. I'll make a note to add some server-side observability for this case.
| message: '❌ Failed to release `${{ matrix.package.name }}@v${{ steps.check-package-version.outputs.committed-version }}`! <https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}|View logs>' | ||
| emoji_reaction: 'x' | ||
|
|
||
| build-s3-artifacts: |
Member
There was a problem hiding this comment.
The aws script is racey, so verified this workflow already limits concurrency to 1 👍
dustinbyrne
force-pushed
the
chore/s3-upload-release
branch
from
17859a6 to
a8c6f89
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Customers will be able to pin their posthog-js version (e.g.
"1"). When a request hitsus-assets.i.posthog.com/array/{team_token}/array.js, Django resolves the configured version and serves the JS artifact from S3. The infra side (buckets + OIDC roles) is in posthog-cloud-infra#7255 — this PR adds the write path from the release workflow.Changes
New file:
.github/scripts/upload-posthog-js-s3.shReusable script that:
packages/browser/dist/*.jstos3://{bucket}/{version}/with immutable cache headersversions.jsonmanifest (or starts with[]){"version": "x.y.z", "timestamp": "..."}if not already presentversions.jsonSafety measures:
VERSION— blocks path traversal and shell metacharactersVERSIONread from env var (not a shell argument) to avoid injection via${{ }}interpolationmktemp -dfor temp filesModified:
.github/workflows/release.ymlNew
upload-s3job that runs afterpublish:S3 UploadGitHub environment (matches OIDC subject from infra PR)check-package-versionus-east-1) then EU (eu-central-1)notify-releasednow also waits onupload-s3The Celery task
sync_js_snippet_manifest(runs every 5 min) will pick up the updatedversions.json, compute pointer maps, validate artifacts, update Redis, and purge CDN.Setup needed before first use
I'll need help with this:
AWS_S3_UPLOAD_ROLE_ARN_US— ARN of the US upload roleAWS_S3_UPLOAD_ROLE_ARN_EU— ARN of the EU upload roleRelease info Sub-libraries affected
Libraries affected
Checklist