Skip to content

Conversation

@thompson-tomo
Copy link
Contributor

@thompson-tomo thompson-tomo commented Dec 6, 2025

Avoids #197 and #240 being an issue going forward and resolves if maintainer steps below are done.

This introduces github workflow for managing releases. This workflow is triggered when a tag is added to a commit and performs the following:

  • Set npm package version based on tag value
  • Create npm package
  • Uploads npm package to npm
  • Create github release with npm package attached and auto generated release notes

Required steps by maintainers to enable closure of the 2 issues:

image

followed by the create release notes option:
image
-> Needed for #197

@thompson-tomo thompson-tomo force-pushed the infra/#240_CreateRelease branch 6 times, most recently from 8de0aa1 to c1be012 Compare December 6, 2025 02:16
@thompson-tomo thompson-tomo changed the title Infra: #197/#240 create release using workflows infra: #197/#240 create release using workflows Dec 7, 2025
@thompson-tomo thompson-tomo force-pushed the infra/#240_CreateRelease branch from 7247e50 to 2935046 Compare December 8, 2025 01:42
@thompson-tomo
Copy link
Contributor Author

@PeterDaveHello / @AndrewSouthpaw / @thlorenz thoughts on automating the release process including release note generation?

Note the manual steps to generate old release notes described in description.

@thompson-tomo thompson-tomo force-pushed the infra/#240_CreateRelease branch 2 times, most recently from a0f8d6b to de1bab5 Compare December 26, 2025 01:23
@thompson-tomo thompson-tomo force-pushed the infra/#240_CreateRelease branch from de1bab5 to dc354bf Compare December 26, 2025 01:24
"name": "doctoc",
"description": "Generates TOC for markdown files of local git repo.",
"version": "2.2.0",
"version": "0.0.0-development",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if this is a good idea, as based on my limited experience with Node.js app development, it doesn't appear to be a common practice.

Copy link
Contributor Author

@thompson-tomo thompson-tomo Dec 28, 2025

Choose a reason for hiding this comment

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

For me it is better and more efficient to use a clear placeholder value rather than having to either manually maintain the version value or adding more advanced ci pipelines given low volume of releases. Note this value is used purely for informational purposes as the git tag value is used to ensure they are aligned.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't love this either. It's standard practice for the "version" tag to accurately reflect the npm and git tags. Not having that can break standard expectations.

What would it take to include bumping the version tag here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is still the case that the package version & git tags all match because at build time the value of the tag is set as the value. This means as a user they see no difference.

The difficulty is if we have the trigger of a release being that a tag is added, how can we get that value to be set in the source code so that it matches. It would mean the tag is 1 commit behind.

The common way to do it would be to raise a pr which bumps the version and when merged the release occurs. That would be a big change for such infrequent releases.

Copy link
Collaborator

@AndrewSouthpaw AndrewSouthpaw Jan 28, 2026

Choose a reason for hiding this comment

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

Ah, ok. I'm fine with it then as long as the end result has them match.

I would've gone the route of raising a PR to bump the version anyway, but this is good enough, and as you said the releases aren't that frequent.

Can you add some notes to the release.yml summarizing the tradeoffs and link to this discussion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added some comments and will add them here if happy.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces automated release management through GitHub workflows to prevent manual release inconsistencies. The workflow triggers on version tags and automates npm package publishing and GitHub release creation.

Key changes:

  • Sets package.json version to development placeholder to be updated by workflow
  • Adds GitHub workflow that publishes to npm and creates releases on tag push
  • Requires maintainer setup of npm trusted publishers and retroactive release creation

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

File Description
package.json Version changed to development placeholder for workflow automation
.github/workflows/release.yml New workflow automating npm publish and GitHub release on version tags

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- run: npm publish

- name: Release
if: ${{ always() }}
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

Using always() condition means the release will be created even if npm tests fail or npm publish fails. This could result in GitHub releases for code that failed testing or wasn't successfully published to npm. Consider using if: success() instead to only create releases when all previous steps succeed.

Suggested change
if: ${{ always() }}
if: ${{ success() }}

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@thompson-tomo thompson-tomo Dec 28, 2025

Choose a reason for hiding this comment

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

Agree this should ideally be changed but will wait for after trusted publishing has been confirmed as we want for the time being that if npm upload fails the artifact's is still on the github release.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good. Trusted publishing is confirmed.

Co-authored-by: Copilot <[email protected]>
"name": "doctoc",
"description": "Generates TOC for markdown files of local git repo.",
"version": "2.2.0",
"version": "0.0.0-development",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't love this either. It's standard practice for the "version" tag to accurately reflect the npm and git tags. Not having that can break standard expectations.

What would it take to include bumping the version tag here?

Co-authored-by: Andrew Smith <[email protected]>
@thompson-tomo
Copy link
Contributor Author

@AndrewSouthpaw how can we continue moving this forward as I am keen to get a release of both libraries to unblock other projects ie otel I contribute to.

Copy link
Collaborator

@AndrewSouthpaw AndrewSouthpaw left a comment

Choose a reason for hiding this comment

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

Just a couple final notes and we're good to go! Thanks for your patience, I was on vacation (again, I've been unemployed and enjoying the time away from a computer).

@thompson-tomo
Copy link
Contributor Author

Thanks for the update Andrew. No worries, I have made the changes in the other repo and will add them here once it is merged. 🙂 also thank you for being willing to review/merge the pr's. 👍

@AndrewSouthpaw
Copy link
Collaborator

Happy to do it. I know from experience that dealing with slow-moving-but-popular repos can be a pain, and I appreciate your persistence in poking us! As long as you keep @'ing me, it'll show up in my email and I'll try to be more consistent reviewing your PRs. 😄

Copy link
Collaborator

@AndrewSouthpaw AndrewSouthpaw left a comment

Choose a reason for hiding this comment

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

This looks good! I'll go ahead and leave this unmerged for you to backport the comments and then I'll land this one too.

Added issue creation step for npm publish failures.
@thompson-tomo
Copy link
Contributor Author

Sure thing, will keep that in mind and keep trying to keep them small. Note I have been trying to clean things up hence the updating/closing out of old issues/pr's and why I raised #303 today.

The workflow changes have been brought across and should be mergable now.

- run: npm publish

- name: Release
if: ${{ always() }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good. Trusted publishing is confirmed.

@AndrewSouthpaw AndrewSouthpaw merged commit bbe3b17 into thlorenz:master Jan 29, 2026
5 checks passed
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.

3 participants