Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge "Build and Test" and "Publish" release jobs #12818

Merged

Conversation

nicolo-ribaudo
Copy link
Member

Q                       A
Fixed Issues? Fixes #1, Fixes #2
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This is what @JLHwung suggested in #12810 (comment).

My original response:

The reason I split the jobs is because the "Publish release on npm" job has access to our npm token, so I wanted to run as few things as possible in it.

Currently it's only running yarn release-tool publish --yes, which means that it only runs code bundled ahead of time in this repository.


The release action takes 7 mins to upload a 3MB artifact (https://github.com/babel/babel/runs/1928439259?check_suite_focus=true): we can save 50% of the time if we merge those two jobs.

However, I'm not comfortable with running make build with the npm token accessible because it would be easily stolen by a malicious nested dependency. I'm marking this PR as draft until I figure out the security model.

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 983f882:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@JLHwung
Copy link
Contributor

JLHwung commented Feb 18, 2021

Yeah the cache action will suffer in IO intensive scenarios: You can generate runtime on the fly. https://github.com/babel/babel/pull/12002/files#r476631461

@nicolo-ribaudo
Copy link
Member Author

I actually think the token probably is safe and this can land as-id, I need to do some tests.

@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review February 18, 2021 16:54
@nicolo-ribaudo
Copy link
Member Author

The secret is not accessible unless we explicitly enable it with

        env:
          YARN_NPM_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}

so make prepublish cannot access it.

@babel-bot
Copy link
Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/40889/

@nicolo-ribaudo nicolo-ribaudo merged commit 1387d66 into babel:main Feb 19, 2021
@nicolo-ribaudo nicolo-ribaudo deleted the same-job-for-build-and-publish branch February 19, 2021 15:00
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 22, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue repo automation 🤖
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants