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: manifest-bump-ci #54

Closed
wants to merge 2 commits into from
Closed

feat: manifest-bump-ci #54

wants to merge 2 commits into from

Conversation

Anush008
Copy link
Member

@Anush008 Anush008 commented Apr 30, 2023

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Description

This PR adds a manifest.json version bump job to .github/workflows/release.yml CI.
After the sematic-release job, the bumped release version is retrieved from package.json and automatically set in manifest.json with the author of the commit being set to the original author of the release.

Note:

https://github.com/open-sauced/browser-extensions/blob/508acea546a0843dd7c5cf4b209cc9d91606a34b/.github/workflows/release.yml#L108

Piping the author info to xargs ensures removal of any whitespace or leading characters. This is optional and can be removed.

Test runs of this updated workflow can be found here. https://github.com/Anush008/browser-extensions/actions.

Related Tickets & Documents

Resolves #53.

Mobile & Desktop Screenshots/Recordings

Not applicable.

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentation?

  • πŸ“œ README.md
  • πŸ““ docs.opensauced.pizza
  • πŸ• dev.to/opensauced
  • πŸ“• storybook
  • πŸ™… no documentation needed

@Anush008 Anush008 requested review from diivi, 0-vortex and bdougie April 30, 2023 06:12
Copy link

@0-vortex 0-vortex left a comment

Choose a reason for hiding this comment

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

Using this technique results in needless extra commits and can be used to essentially work around anything.

Please extend the release automation or remove it entirely for a committed semantic release configuration where you don't need to use this complication πŸ™

@Anush008
Copy link
Member Author

Anush008 commented Apr 30, 2023

I understand that this will result in an additional commit for a release, but as the issue states, version bumping of the manifest.json is unique to this particular browser extension project. Therefore, incorporating this feature into the release system would not provide any added benefits to other projects utilizing it.

Additionally, replacing this well-setup release system used across other projects with something else solely for this one project does not seem appropriate or necessary at this time.

@0-vortex
Copy link

I understand that this may result in an additional commit for a release, but as the issue states, version bumping of the manifest.json is unique to this particular browser extension project. Therefore, incorporating this feature into the release system would not provide any added benefits to other projects utilizing it.

The release action is not meant to support any other projects than open-sauced. It's a simplification so that we don't install all the semantic release packages in projects and keep the configuration centralised, an example on a better way to make npm tooling portable.

Additionally, replacing this well-setup release system used across other projects with something else solely for this one project does not seem appropriate or necessary at this time.

It is copy pasting one file, once - vs - one extra release workflow committing twice every release. It is totally the best practice to decouple the configuration if upstream does not comply with the requirements or available resources this project has available. If the way we release is different here, it should be visible in the release configuration. Making the workaround happen in another scope also requires additional documentation. Waaaaaay too much work in my opinion.

@Anush008
Copy link
Member Author

Anush008 commented Apr 30, 2023

That sounds right. Now when I think about it, an extra manifest bump commit for every release isn't reasonable. I'd now prefer extending the release system to do the manifest bump and keep it in a single commit. Thanks.

@diivi diivi removed their request for review April 30, 2023 12:06
@Anush008 Anush008 closed this May 1, 2023
@Anush008 Anush008 deleted the feat-manifest-bump-ci branch June 3, 2023 15:46
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.

Feature: Update manifest version after release
2 participants