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

feat: add GitHub action for releasing via Changeset #1029

Closed
wants to merge 5 commits into from
Closed

feat: add GitHub action for releasing via Changeset #1029

wants to merge 5 commits into from

Conversation

ayushnau
Copy link
Contributor

Changes Added.

Added Release github Action using Changeset. 

Related issue(s)

Related to: #1028

@ayushnau ayushnau changed the title feat: Added the release github action for releasing through changeset feat: added the release GitHub action for releasing through Changeset Jun 19, 2024
@ayushnau ayushnau changed the title feat: added the release GitHub action for releasing through Changeset feat: added the release, GitHub action for releasing through Changeset Jun 19, 2024
@ayushnau ayushnau changed the title feat: added the release, GitHub action for releasing through Changeset feat: add GitHub action for releasing via Changeset Jun 19, 2024
@ayushnau
Copy link
Contributor Author

@smoya please review and merge this pr.

package.json Outdated
Comment on lines 42 to 44
"changeset": "changeset",
"version-packages": "changeset version",
"publish-packages": "turbo run build && changeset publish"
Copy link
Member

Choose a reason for hiding this comment

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

instead of adding those to package.json did you consider just using npx and package directly in release config?

in past with semantic-release we resigned from using npm commands if they were not needed during regular development. So semantic-release we used directly like npx [email protected] so we had less things to maintain, it was easier to reuse across repos, and dependabot was not bugging us with new versions bump

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok now added.

# Don't make changes to this file in this repo as they will be overwritten with changes made to the same file in the above-mentioned repo

# It does magic only if there is a package.json file in the root of the project
name: Release - if Node project
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name: Release - if Node project
name: Release with changeset

can you please name file like release-with-changeset.yml as might be that this release will work also in our generator and then also other monorepos, so we will maintain it in .github and we will do it with release-with-changeset.yml name - so once you opt in in future for file from .github, it will just be overwritten

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok sure changed the name as required.

Copy link
Member

@smoya smoya Jun 20, 2024

Choose a reason for hiding this comment

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

Minor: Actually, shouldn't be called changesets in plural? That's the name of the project. So release-with-changesets.yml? On the contrary, the config is located in a directory called .changeset so not sure 🤷 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok changed to the changesets.

@@ -0,0 +1,141 @@
# This action is centrally managed in https://github.com/asyncapi/.github/
Copy link
Member

@smoya smoya Jun 20, 2024

Choose a reason for hiding this comment

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

This is not centrally managed yet. Please remove the comment. We can add it once this is moved into .github repo

Suggested change
# This action is centrally managed in https://github.com/asyncapi/.github/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

you can revert package-lock changes as well

run: npm ci
- if: steps.packagejson.outputs.exists == 'true'
name: Install Changesets
run: npm install --save-dev @changesets/[email protected]
Copy link
Member

Choose a reason for hiding this comment

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

is it still needed if in the end you call below npx ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok removed we should add this like we do for semantic release.

Comment on lines +120 to +121
publish: npx turbo run build && npx changeset publish
version: npx changeset version
Copy link
Member

Choose a reason for hiding this comment

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

if you do not specify the version, npx will always pick up latest - which in future will kick maintainers big time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok now added the version.

Comment on lines +11 to +12
# The below lines are not enough to have release supported for these branches
# Make sure the configuration of the `semantic-release` package mentions these branches
Copy link
Member

@smoya smoya Jun 20, 2024

Choose a reason for hiding this comment

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

We do not use semantic-release anymore (and projects that will use this workflow won't use it either) so this comment should be removed.

Suggested change
# The below lines are not enough to have release supported for these branches
# Make sure the configuration of the `semantic-release` package mentions these branches

Comment on lines +13 to +18
- next-spec
- next-major
- next-major-spec
- beta
- alpha
- next
Copy link
Member

@smoya smoya Jun 20, 2024

Choose a reason for hiding this comment

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

Do we need anything else to be configured (perhaps in changeset config file?) to allow those branches to be released as well? How does it work?

cc @derberg @KhudaDad414

Copy link
Member

@smoya smoya Jun 20, 2024

Choose a reason for hiding this comment

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

I found out this in their docs: https://github.com/changesets/changesets/blob/main/docs/prereleases.md

It should be done manually it seems. I guess in the workflow, detecting we are not in master branch and run npx changeset pre enter {tag} then. More investigation is needed.

We can work on it in a second iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok sure will do that.

@smoya
Copy link
Member

smoya commented Jun 20, 2024

I don't fully understand why we created this in a separate PR. The fact we move to changesets is purely linked to the monorepo needs. If something goes wrong, I expect to revert just one commit and not few.

Please either add the changes into #1008 or rather change the base branch of this PR to target the one in #1008 so we merge it into that branch.

@smoya
Copy link
Member

smoya commented Jun 21, 2024

@ayushnau I understand this PR can be closed as you moved the changes to #1008 ?

@smoya smoya closed this Jul 5, 2024
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.

4 participants