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(script): handle patch versions after the .0 for set version #36020

Closed
wants to merge 2 commits into from

Conversation

kelset
Copy link
Contributor

@kelset kelset commented Jan 31, 2023

Summary

A small backport to main of a local fix done in 0.71 to account for the logic for releases 0.Y.1,2,3-prerelease (meaning, not just strictly 0).

I could have done like the other logics and just remove the check for patch, but decided to at least make sure it's a digit 😅

Changelog

[INTERNAL] [FIXED] - handle patch versions after the .0 for set version

Test Plan

Tested in 0.71-stable, without it we can't test RNTestProject.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. labels Jan 31, 2023
@rshest
Copy link
Contributor

rshest commented Jan 31, 2023

I think there are failing tests that are related to this change, e.g. https://app.circleci.com/pipelines/github/facebook/react-native/19246/workflows/8c6b4a43-0304-4bd6-9069-1747e5b18242/jobs/429271/parallel-runs/0/steps/0-107

Version 0.20.3-rc-0 is not valid for dry-runs

Copy link
Contributor

@rshest rshest left a comment

Choose a reason for hiding this comment

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

Please see my comment above, it looks like it breaks the tests.

@cipolleschi
Copy link
Contributor

As @rshest said, you need to update also the tests to take into account this possibility.

Also, why do we want to support prerelease like 0.71.1-rc.0? What's the use case for this? 🤔

@kelset
Copy link
Contributor Author

kelset commented Feb 1, 2023

hey @cipolleschi / @rshest - thanks for your feedback, apologises for not running yarn test locally before opening the PR (I usually don't do it because it's way more local errors then when running on CI).

So, for the tests that are broken - there's a long and short version. The short version is that I removed them because they were "blocking" completely valid scenarios.

The long story is this:

  • first off, as mentioned, scenarios like 0.20.3-rc-0 are entirely valid semver cases, along the lines of >1.2.3-alpha.3. You can read more on Prerelease Tags here
  • just to be clear, what happens when you add a -something-something to a version is that it's automatically a prerelease, so ex. if you have the non prerelease out 0.20.3-rc-0 and 0.20.3, in the vast majority of cases it will pick 0.20.3 (read more here) - this to say that it's not a massive problem to have releases out that fall into that pattern (0.20.3-rc-0)
  • the fact that we, in RN core, we don't use the vast majority of these prereleases but only usually do it in the RC phase before 0.X.0 is just a "decision" - we could in the future or in very specific cases rely on the -prerelease alternative (maybe as a way of testing new samsung patches? that'd be an interesting use case to explore)
  • we rely on the prerelease pattern for doing the local testing before publishing a new release, especially for testing a fresh new project where we generate a local variation of what lives in the branch in the form of 0.71.1-20230201-1157 (basically appending the timestamp); this means that we need to be able to do 0.X.Y-timestamp (that's what I needed to fix via this PR)

And because of all the above, basically, the correct thing to do to me was to simply remove these tests - 'cause the only way I could think of "inverting" them would turn them into basically redundant tests (verifying that all the parts are parsed correctly, that's already tested)


if there's some variation of the tests that you'd like me to add instead do let me know, happy to change

@kelset kelset requested a review from rshest February 1, 2023 13:34
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,463,813 -315
android hermes armeabi-v7a 7,784,268 -1,219
android hermes x86 8,936,997 -958
android hermes x86_64 8,795,086 -1,088
android jsc arm64-v8a 9,648,959 -703
android jsc armeabi-v7a 8,383,226 -1,597
android jsc x86 9,710,923 -1,350
android jsc x86_64 10,187,777 -1,460

Base commit: bf34810
Branch: main

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Feb 7, 2023
@facebook-github-bot
Copy link
Contributor

@cipolleschi merged this pull request in 7adf6b1.

@kelset kelset deleted the kelset/fix-script-release-logic branch February 7, 2023 12:16
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…book#36020)

Summary:
A small backport to main of a local fix done in 0.71 to account for the logic for releases 0.Y.1,2,3-prerelease (meaning, not just strictly 0).

I could have done like the other logics and just remove the check for patch, but decided to at least make sure it's a digit 😅

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[INTERNAL] [FIXED] - handle patch versions after the .0 for set version

Pull Request resolved: facebook#36020

Test Plan: Tested in 0.71-stable, without it we can't test RNTestProject.

Reviewed By: jacdebug, cortinico

Differential Revision: D42924375

Pulled By: cipolleschi

fbshipit-source-id: b003d884cc45a2602adbc14fa8b66d3f1e0c94a6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants