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

ci: publish workflow #2591

Closed
wants to merge 6 commits into from
Closed

ci: publish workflow #2591

wants to merge 6 commits into from

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 commented Dec 28, 2023

This is a GitHub action workflow that let's maintainers of Faker run a npm publish process, so that the package @faker-js/faker will be published to npm registry.

It is respecting the version and dist-tag by reading the package.json.

The process will be:

  1. Someone create a normal release PR
  2. The release PR gets merged into next branch
  3. Someone hits the publish workflow in the github actions menu // <- this was done locally before
  4. After the release succeeded, someone will normally create the GitHub Release

It requires that one of the maintainers create a new access token on NPM for the @faker-js/faker package, or for the @faker/* orga-scope.
This then needs to be configured in GitHub as a secret with the name NPM_AUTH_TOKEN, so it can be picked up by the CI.

Note that publish will run the prepublishOnly script that is configured in the package.json

This workflow will NOT "craft" a release, nor will it create tags/releases on github.
It just and only automates the publishing to npm.

Hint: the workflow is tested for 3 months now in https://github.com/salsita/node-pg-migrate

@Shinigami92 Shinigami92 added the c: infra Changes to our infrastructure or project setup label Dec 28, 2023
@Shinigami92 Shinigami92 self-assigned this Dec 28, 2023
Copy link

codecov bot commented Dec 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (c2d7e29) to head (66e6179).
Report is 1 commits behind head on next.

Current head 66e6179 differs from pull request most recent head 0191f87

Please upload reports for the commit 0191f87 to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2591   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files        2989     2989           
  Lines      216081   216081           
  Branches      953      951    -2     
=======================================
  Hits       216008   216008           
  Misses         73       73           

see 1 file with indirect coverage changes

Copy link

netlify bot commented Jun 21, 2024

Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit 0191f87
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/6679a9b31bbe6700089ca434
😎 Deploy Preview https://deploy-preview-2591.fakerjs.dev
📱 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 site configuration.

@Shinigami92 Shinigami92 force-pushed the publish-via-ci branch 2 times, most recently from b6ef4c9 to c8e84b2 Compare June 21, 2024 10:13
@Shinigami92 Shinigami92 added this to the vAnytime milestone Jun 21, 2024
@Shinigami92 Shinigami92 marked this pull request as ready for review June 21, 2024 10:19
@Shinigami92 Shinigami92 requested a review from a team as a code owner June 21, 2024 10:19
@Shinigami92 Shinigami92 requested review from ST-DDT, xDivisionByZerox and a team June 21, 2024 10:19
.github/workflows/publish.yml Outdated Show resolved Hide resolved
.github/workflows/publish.yml Show resolved Hide resolved
Shinigami92 and others added 2 commits June 21, 2024 15:33
@Shinigami92 Shinigami92 requested a review from ST-DDT June 21, 2024 13:39
ST-DDT
ST-DDT previously approved these changes Jun 21, 2024
Comment on lines +38 to +44
PACKAGE_DIST_TAG=$(node -e "console.log(/^\d+\.\d+\.\d+(\-(\w+)\.\d+)$/.exec(require('./package.json').version)?.[2] || 'latest')")
pnpm publish --access public --tag $PACKAGE_DIST_TAG

- name: Set next dist-tag
run: |
PACKAGE_VERSION=$(node -e "console.log(require('./package.json').version)")
pnpm dist-tag add @faker-js/faker@$PACKAGE_VERSION next
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should tag it with next by default and set the latest only when it is a stable release:

Suggested change
PACKAGE_DIST_TAG=$(node -e "console.log(/^\d+\.\d+\.\d+(\-(\w+)\.\d+)$/.exec(require('./package.json').version)?.[2] || 'latest')")
pnpm publish --access public --tag $PACKAGE_DIST_TAG
- name: Set next dist-tag
run: |
PACKAGE_VERSION=$(node -e "console.log(require('./package.json').version)")
pnpm dist-tag add @faker-js/faker@$PACKAGE_VERSION next
PACKAGE_VERSION=$(node -e "console.log(require('./package.json').version)")
pnpm publish --access public --tag next
if [[ "$PACKAGE_VERSION" != *"-"* ]]; then
pnpm dist-tag add @faker-js/faker@$PACKAGE_VERSION latest
fi

Not tested.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we are extracting the dist-tag out of the version field and then e.g. alpha, beta or whatever is used
This is default tagging behavior guided to semver

Copy link
Member

@ST-DDT ST-DDT Jun 24, 2024

Choose a reason for hiding this comment

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

confused: But we don't have alpha or beta tags!? Or did we remove them after stable release?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh good that we found out that we misused that before 🫤

just some examples that use e.g. beta dist-tags (which are not my own packages)

env:
NPM_AUTH_TOKEN: ${{ secrets.NPM_AUTH_TOKEN }}

- name: Publish
Copy link
Member

Choose a reason for hiding this comment

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

According to our release checklist, we should remove the "next branch warning" before the publish.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what this has to do with this PR 🤔

Copy link
Member

@ST-DDT ST-DDT Jun 24, 2024

Choose a reason for hiding this comment

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

confused: IMO This PR is intended to partially automate the release process, so it should actually contain the same steps as the manual workflow?!
If we don't, we have to permanently remove that section from the readme.

Copy link
Member Author

Choose a reason for hiding this comment

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

wait, we ever did remove that banner for a release?! I was not aware of that
if I e.g. look in https://www.npmjs.com/package/@faker-js/faker/v/8.4.1 or https://www.npmjs.com/package/@faker-js/faker/v/9.0.0-alpha.0 we did not do that
Only in the newest release and I was not really part of that

IMO we should just remove that banner completely (in a separate PR using docs: *)
But if you want to you can also leave this open until thursday

Copy link
Member

Choose a reason for hiding this comment

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

FFR: The comment was added for this issue:

Copy link
Member Author

Choose a reason for hiding this comment

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

After a not so good sleep last night, I decided (at least for my self) that I'm not interested in manipulating repository code from a workflow and so also dont like to work further on that topic especially not from this PR, as the scope and intend was something different from my side.
I just wanted to provide a workflow that can be used to release the "current" state of next branch without having to use credentials manually.

So feel free to either provide a follow-up PR (my preference) or jump into this PR and take it over (not my preference).

@ST-DDT ST-DDT added p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision labels Jun 24, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Jul 2, 2024

Superseded by #2991 as requested here.

@ST-DDT ST-DDT closed this Jul 2, 2024
@ST-DDT ST-DDT deleted the publish-via-ci branch July 2, 2024 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: infra Changes to our infrastructure or project setup p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants