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

Fix rare issue with canary id from git sha #2448

Merged

Conversation

MichaelRyanWebber
Copy link
Contributor

In the rare case that the first 7 characters of a git sha contains only numbers and begins with 0, SemVer will reject the canary (due to this part of the canaryIdentifier) as an invalid version.

In such cases, make sure we include enough of the sha to get a letter.

Ignore the ~1:1.5billion chance that SHA is all numbers starting with 0, but optionally the sha could be replaced with the string jackpot

Todo:

  • Add tests

Change Type

Indicate the type of change your pull request is:

  • documentation
  • patch
  • minor
  • major

…ly numbers and begins with 0, SemVer will reject the canaryIdentifier. In such cases, make sure we include enough of the sha to get a letter.
@MichaelRyanWebber
Copy link
Contributor Author

MichaelRyanWebber commented Apr 2, 2024

@hipstersmoothie Not sure how you'd actually want to solve this case, but figured I'd throw an attempt at it

could also do lte(next, currentVersion, true) here to get the "loose" checking, but I'd guess there would be issues later since "full" is the default.

the two regexes for reference:

full /^v?(0|[1-9]\d{0,256})\.(0|[1-9]\d{0,256})\.(0|[1-9]\d{0,256})(?:-((?:0|[1-9]\d{0,256}|\d{0,256}[a-zA-Z-][a-zA-Z0-9-]{0,250})(?:\.(?:0|[1-9]\d{0,256}|\d{0,256}[a-zA-Z-][a-zA-Z0-9-]{0,250}))*))?(?:\+([a-zA-Z0-9-]{1,250}(?:\.[a-zA-Z0-9-]{1,250})*))?$/
loose /^[v=\s]*(\d{1,256})\.(\d{1,256})\.(\d{1,256})(?:-?((?:\d{1,256}|\d{0,256}[a-zA-Z-][a-zA-Z0-9-]{0,250})(?:\.(?:\d{1,256}|\d{0,256}[a-zA-Z-][a-zA-Z0-9-]{0,250}))*))?(?:\+([a-zA-Z0-9-]{1,250}(?:\.[a-zA-Z0-9-]{1,250})*))?$/

@hipstersmoothie
Copy link
Collaborator

@MichaelRyanWebber this looks great! such an odd issues to hit. my only note is a comment here would be nice

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 80.55%. Comparing base (b8d1af6) to head (c214a53).
Report is 4 commits behind head on main.

Files Patch % Lines
packages/core/src/auto.ts 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2448      +/-   ##
==========================================
- Coverage   80.63%   80.55%   -0.08%     
==========================================
  Files          69       69              
  Lines        5680     5683       +3     
  Branches     1335     1336       +1     
==========================================
- Hits         4580     4578       -2     
- Misses        718      719       +1     
- Partials      382      386       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hipstersmoothie
Copy link
Collaborator

Once you add the comment I'll merge

@MichaelRyanWebber
Copy link
Contributor Author

@hipstersmoothie added a comment. lmk if that seems clear enough!

@hipstersmoothie hipstersmoothie added the patch Increment the patch version when merged label Apr 3, 2024
@hipstersmoothie hipstersmoothie merged commit 666e239 into intuit:main Apr 3, 2024
5 of 9 checks passed
@hipstersmoothie
Copy link
Collaborator

It's out 🎉

@laughedelic
Copy link
Contributor

Hi @hipstersmoothie, release of this changes doesn't have the binaries attached as assets: https://github.com/intuit/auto/releases/tag/v11.1.3
Can you fix it or release another version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants