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

Migrate publish to gh actions #1312

Closed
wants to merge 1 commit into from
Closed

Conversation

vzaidman
Copy link
Contributor

@vzaidman vzaidman commented Aug 6, 2024

2/2 commit converting Circle CI to Github actions.

Removed Circle CI to ensure we don't try to publish a version twice.

  • Added the "deploy" step that only runs for tag pushes
  • It waits for testing and validation, and fails if either of them fails
  • This PR uses --dry-run to ensure no publish happens unexpectedly. This arg will be removed in a subsequent PR.

Successful CI with no deployment step when a PR is updated-
https://github.com/facebook/metro/actions/runs/10269830075
Screenshot 2024-08-06 at 16 54 20

Successful DRY RUN deploy from a hotfix branch (works pretty much the same on main)-
https://github.com/facebook/metro/actions/runs/10269576329/job/28415382215
Screenshot 2024-08-06 at 16 49 21

Deploy fails for a wrongly formatted tag (used tag "bad-tag-name") with a clear error-
https://github.com/facebook/metro/actions/runs/10269509288/job/28415186326
Screenshot 2024-08-06 at 16 29 19

Deploy fails for a tag pushed from a branch not following the Metro's release branch naming conventions
Screenshot 2024-08-06 at 17 01 28

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 6, 2024
@vzaidman vzaidman force-pushed the migrate-publish-to-gh-actions branch 2 times, most recently from 75f0209 to 6025147 Compare August 6, 2024 12:06
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.91%. Comparing base (e3aaa0b) to head (6025147).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1312   +/-   ##
=======================================
  Coverage   83.91%   83.91%           
=======================================
  Files         208      208           
  Lines       10942    10942           
  Branches     2752     2752           
=======================================
  Hits         9182     9182           
  Misses       1760     1760           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vzaidman vzaidman force-pushed the migrate-publish-to-gh-actions branch 2 times, most recently from 8306e95 to 7a03b2c Compare August 6, 2024 15:52
@vzaidman vzaidman requested review from robhogan and huntie August 6, 2024 16:03
@vzaidman vzaidman force-pushed the migrate-publish-to-gh-actions branch from 7a03b2c to eaa41dc Compare August 6, 2024 16:10
.github/scripts/publish.sh Outdated Show resolved Hide resolved
.github/scripts/publish.sh Outdated Show resolved Hide resolved
Comment on lines 12 to 17
# head_ref is per PR, so this ensures that updating the latest PR commit
# will cancel the previous run of the workflow and trigger a new one
concurrency:
group: "build-test-and-deploy-${{ github.head_ref }}"
cancel-in-progress: true
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the effect of this if the trigger is a tag push?

Copy link
Contributor Author

@vzaidman vzaidman Aug 7, 2024

Choose a reason for hiding this comment

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

Good point. Updated to github.ref that is unique per tag/pr and updated the comment

steps:
- uses: actions/checkout@v4
- uses: ./.github/actions/yarn-install
- run: echo "//registry.npmjs.org/:_authToken=${{ secrets.NPM_TOKEN }}" >> ~/.npmrc
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to check, will NPM_TOKEN be exposed to the code under test at all, e.g. as an env var? Is this secure e.g. against third-party PRs that modify the workflow yaml itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As opposed to Circle CI the GH Actions default to be available to this top level yml only. Any secrets that are needed in actions (such as actions/checkout@v4 or even ./.github/actions/yarn-install above) has to be passed explicitly by passing them as action input or by passing using secrets: inherit to the job

.github/scripts/publish.sh Outdated Show resolved Hide resolved
@vzaidman vzaidman force-pushed the migrate-publish-to-gh-actions branch 8 times, most recently from 043b47c to 38381d1 Compare August 8, 2024 12:08
@facebook-github-bot
Copy link
Contributor

@vzaidman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary:
2/2 commit converting Circle CI to Github actions.

Removed Circle CI to ensure we don't try to publish a version twice.

* Added the "deploy" step that only runs for tag pushes
* It waits for testing and validation, and fails if either of them fails
* This PR uses `--dry-run` to ensure no publish happens unexpectedly. This arg will be removed in a subsequent PR.

Pull Request resolved: #1312

Test Plan:
Successful CI with no deployment step when a PR is updated-
https://github.com/facebook/metro/actions/runs/10269830075
 {F1796157419}

Successful **DRY RUN** deploy from a hotfix branch (works pretty much the same on main)-
https://github.com/facebook/metro/actions/runs/10269576329/job/28415382215
 {F1796157706}

Deploy fails for a wrongly formatted tag (used tag "bad-tag-name") with a clear error-
https://github.com/facebook/metro/actions/runs/10269509288/job/28415186326
{F1796157882}

Deploy fails for a tag pushed from a branch **not following** the Metro's release branch naming conventions
 {F1796158249}

Differential Revision: D60961268

Pulled By: vzaidman
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D60961268

@vzaidman vzaidman force-pushed the migrate-publish-to-gh-actions branch from 38381d1 to 501b149 Compare August 8, 2024 12:24
@robhogan
Copy link
Contributor

robhogan commented Aug 8, 2024

LGTM, thanks!

@facebook-github-bot
Copy link
Contributor

@vzaidman merged this pull request in bccea34.

@vzaidman vzaidman deleted the migrate-publish-to-gh-actions branch August 30, 2024 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants