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

Check primer view components versions consistency on commit #17706

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

cbliard
Copy link
Member

@cbliard cbliard commented Jan 23, 2025

So that I'll stop upgrading it in Gemfile.lock and not in frontend/package.json. See #17681

@myabc
Copy link
Contributor

myabc commented Jan 23, 2025

@cbliard 👍🏻 definitely a good idea! - do you think this is best as a commit check or as a workflow check?*

  • FWIW I'm (personally) not a big fan of commit hooks - I am constantly interactive rebasing, editing and squashing commits and when I've tried installing git commit hooks in the past, they really got in the way of my workflow. that said, everyone has a slightly different workflow, and setting up git hooks is an optional step for our team.

echo "Checking for same primer view components version in Gemfile.lock and frontend/package.json"

versions=$(grep --color=never primer.view.components frontend/package.json Gemfile.lock \
| grep --extended-regexp --only-matching '\d+\.\d+\.\d+' \
Copy link
Contributor

Choose a reason for hiding this comment

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

@cbliard this would obviously break if we were to do a minor release without the patch level component, e.g. 0.53. @HDinger do we always include the patch level (0) when we do major/minor releases?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would also not work with pre-release versions such as 1.2.3-alpha , however I'm not sure if we ever to pre releases.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are currently publishing canary versions - see workflow here: https://github.com/opf/primer_view_components/actions/runs/12822373410/job/35755182385

🦋  info Publishing "@openproject/primer-view-components" at "0.0.0-20250117035659"
🦋  success packages published successfully:
🦋  @openproject/[email protected]

Whether or not we're actively using the canary versions while developing core features, I'm not sure (I've been working mostly linearly so far, so haven't had a need to do this yet).

@HDinger @bsatarnejad do you know? Do you always release primer-view-components before updating call sites in core (as I did with #17569).

Copy link
Contributor

Choose a reason for hiding this comment

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

As an aside: (this probably belongs in a wiki rather than PR comment)

the "escape hatch" for local dev is changing the dependency version in package.json to the local repo (e.g. "@openproject/primer-view-components": "file:../../primer_view_components"). Bundler does this better by letting you configure a local path override: bundle config local.openproject-primer_view_components=../../primer_view_components

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it follows semantic versioning.
And it won't break, it would just go unnoticed and print something like this:

Ok. Primer view components version is  everywhere

version is not detected

I could amend it slightly to make the message different if version can't be detected, but frankly that's overkill.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cbliard this would obviously break if we were to do a minor release without the patch level component, e.g. 0.53. @HDinger do we always include the patch level (0) when we do major/minor releases?

Yes, we do

Copy link
Contributor

Choose a reason for hiding this comment

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

@HDinger @bsatarnejad do you know? Do you always release primer-view-components before updating call sites in core (as I did with #17569).

usually we do it like that, yes. I can imagine, that we want to try things with the canary versions locally but usually not in production.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @myabc I always release Primer-view-components first, then update core with new version both in front-end and backend. Afterward, I make the necessary changes in the relevant places.

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 updated the script to include the pre-release version if any (the "pre.3-rt12" part in 1.2.3-pre.3-rt12 version), and also to have a better message if version cannot be detected.

Copy link
Contributor

@dombesz dombesz left a comment

Choose a reason for hiding this comment

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

Works nicely, thanks @cbliard !

@cbliard
Copy link
Member Author

cbliard commented Jan 24, 2025

👍🏻 definitely a good idea! - do you think this is best as a commit check or as a workflow check?*

Maybe a workflow check, but a commit check is cheaper and faster to develop. I just wanted to write one for me, and found it selfish to keep it private, hence this PR.

  • FWIW I'm (personally) not a big fan of commit hooks - I am constantly interactive rebasing, editing and squashing commits and when I've tried installing git commit hooks in the past, they really got in the way of my workflow. that said, everyone has a slightly different workflow, and setting up git hooks is an optional step for our team.

Sure. That's why commit hooks are an option.
Lefthook is nice as it runs only if specified files have been changed, and it runs commands in parallel.

So that I'll stop upgrading it in `Gemfile.lock` and not in
`frontend/package.json`. See #17681
@cbliard cbliard force-pushed the ensure-primer-view-components-version-is-the-same-everywhere branch from cabc09c to b034388 Compare January 24, 2025 07:59
@cbliard
Copy link
Member Author

cbliard commented Jan 24, 2025

Thanks for your suggestions @myabc. I pushed an updated version.
Could you review it again please?

@cbliard cbliard requested a review from myabc January 24, 2025 08:16
Copy link
Contributor

@myabc myabc 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 making the changes @cbliard!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants