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

Regenerate release notes on release updates #497

Merged
merged 6 commits into from
Feb 20, 2025

Conversation

rantoniuk
Copy link
Contributor

@rantoniuk rantoniuk commented Feb 14, 2025

Fixes #474

@ncipollo:

  1. I'm not sure why the dist/* is part of the repo, since the build/release should be out of here. Let me know if you what me to remove dist/* from the PR (but I had to have it for testing - yes, it works ;-) ).
  2. The logic is so complicated that I didn't know if I should use omitBodyDuringUpdate somewhere or not and what to do in case someone sets body explicitly.
  3. yarn release is not working, since it's depending on ncc (not in deps) and ts-node that is not in --production.

Copy link
Owner

@ncipollo ncipollo left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Had a few questions and change requests.

@rantoniuk rantoniuk force-pushed the fix-releasenotes-gen branch 4 times, most recently from a49f693 to 28e73c0 Compare February 15, 2025 19:00
@rantoniuk rantoniuk force-pushed the fix-releasenotes-gen branch from 28e73c0 to 0a1e176 Compare February 17, 2025 12:42
@rantoniuk rantoniuk force-pushed the fix-releasenotes-gen branch from 0a1e176 to 7cc8991 Compare February 17, 2025 12:47
@rantoniuk rantoniuk requested a review from ncipollo February 17, 2025 13:06
@ncipollo
Copy link
Owner

ncipollo commented Feb 17, 2025

Sorry - would you mind rebasing now 😅. I think it's a good idea to add formatting rules to the project, but I wanted to split that out from this change. I checked in the biome rules on main and ran the formatter. For some reason there are some conflicts, even though I copied the biome file from this branch.

* main:
  Attempt ncipollo#1 at fixing workflow file
  Add initial releases workflow
  Add biome and initial format rules
@rantoniuk
Copy link
Contributor Author

rantoniuk commented Feb 17, 2025

Done. Still, the yarn release command is broken, it will not run in --production mode with the current package.json setup:

$ rm -rf node_modules

$ yarn release
yarn run v1.22.22
$ yarn clean && yarn install --production && yarn build && yarn package
$ rm -rf lib/*
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
[3/5] 🚚  Fetching packages...
[4/5] 🔗  Linking dependencies...
[5/5] 🔨  Building fresh packages...
$ tsc
/bin/sh: tsc: command not found
error Command failed with exit code 127.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 127.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Yes, you guessed correctly, the dist/ in this PR is not built using --production but a standard yarn install from development mode after running yarn test.

(before you say: try doing npm install -g typescript - no, global installations are the wrong way to go and they will cause problems when working on multiple projects with different TS versions. That's why npx was created for or rollup like in the parent repo )

@rantoniuk
Copy link
Contributor Author

Thanks for triggering the tests! Do you need anything else?

@rantoniuk rantoniuk requested a review from ncipollo February 19, 2025 22:27
Copy link
Owner

@ncipollo ncipollo left a comment

Choose a reason for hiding this comment

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

Looks great 🎉. Thank you for bearing with me through my PR feedback haha. Thanks for this contribution!!

@ncipollo ncipollo merged commit a8c5a7f into ncipollo:main Feb 20, 2025
2 checks passed
@rantoniuk
Copy link
Contributor Author

rantoniuk commented Feb 21, 2025

@ncipollo I guess you still need to re-push @v1 for this to be visible via ncipollo/release-action@v1 ?

@ncipollo
Copy link
Owner

Yeah I need push the next minor tag and then update v1 to point to that. I will get to that this weekend.

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.

Draft update clears the release notes body
2 participants