-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Improve version checks to avoid mistakes in the versioning #35296
Conversation
This pull request was exported from Phabricator. Differential Revision: D41161756 |
…35296) Summary: Pull Request resolved: facebook#35296 This change adds some version checks and enforces that every version matches some specific format based on the build type we are trying to run. ## Changelog [General][Changed] - Improve version checks Differential Revision: D41161756 fbshipit-source-id: da7dbfbf8685f8d437a310a3c305250eb56dd89b
461485a
to
dd58810
Compare
This pull request was exported from Phabricator. Differential Revision: D41161756 |
scripts/run-ci-e2e-tests.js
Outdated
if ( | ||
exec( | ||
'node ./scripts/set-rn-version.js --to-version 1000.0.0 --build-type dry-run', | ||
).code | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change makes the CircleCI test_js
and test_js_prev_lts
fails miserably because they can't find ruby anymore.
It doesn't make any sense to me, unless there are different scripts involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you end up figuring out what was going on here? did rebasing on top of main help? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, unfortunately. No idea why this invocation make it fail ruby... 😕
This pull request was exported from Phabricator. Differential Revision: D41161756 |
dd58810
to
50a019c
Compare
Base commit: 5dd0f73 |
…35296) Summary: Pull Request resolved: facebook#35296 This change adds some version checks and enforces that every version matches some specific format based on the build type we are trying to run. ## Changelog [General][Changed] - Improve version checks Differential Revision: D41161756 fbshipit-source-id: 10f5d22f0f1a9049397414c95f95fb8b6f5332a8
50a019c
to
7833857
Compare
This pull request was exported from Phabricator. Differential Revision: D41161756 |
Base commit: cb7f1b1 |
PR build artifact for 7833857 is ready. |
PR build artifact for 7833857 is ready. |
This pull request was exported from Phabricator. Differential Revision: D41161756 |
7833857
to
91d0401
Compare
…35296) Summary: Pull Request resolved: facebook#35296 This change adds some version checks and enforces that every version matches some specific format based on the build type we are trying to run. ## Changelog [General][Changed] - Improve version checks Differential Revision: D41161756 fbshipit-source-id: 7f4caee6a2e4fa895d33bd2ef938352cf8703d27
PR build artifact for 91d0401 is ready. |
PR build artifact for 91d0401 is ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for working on this - it goes in the right direction, I feel we really need to have a clear place in the code & docs that says all the potential versions and where they can be encountered.
Some comments are just questions, I think that once we've addressed it all we'll be able to merge soon.
Sorry it took me so long to loop back on this :(
scripts/version-utils.js
Outdated
function is1000_0_1(version) { | ||
return ( | ||
version.major === '1000' && version.minor === '0' && version.patch === '1' | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this case covering? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this: https://github.com/facebook/react-native/blob/main/.circleci/config.yml#L1646
I think we can change this, but I left it here to highlight this and let you and Nico spread some knowledge about why this value has been chosen! :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that was not created by either of us, looks like it's coming from here e4b5d3e (and I was very much no aware of that)
soooo yeah. I guess we need to keep it around for now? It looks like it's just doing a dry run, so maybe realign it with the versioning for the dry runs? Also, the more I look at that PR, the more it feels wrong that the package_and_publish_release_dryrun
is using a fixed version and not the one set in the branch :/ it means that even when we are about to publish ex. 0.71.0, the dryrun will still run with 1000.0.1 and it will basically "dry run test" something different from the final result 😅
might be worth a refactoring of that part of circleci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I tried to pick up the version from the package.json for this problem, but it can't work. The reason is that the set-rn-version
script fails if we try to set the same version that is already in the package.json
. That's why they used the v1000.0.1
, because they were sure that it would have worked.
We can add a bit of extra logic to dry-run on a patch: this will work for dry-running on main
and on stable
branches. However, it won't be exact for RCs. However, for RCs, it is very hard to properly bump them: the prerelease
part of the version is handled as a string (because sometimes it is a date - for the nightlies -, some other times it is the RC.x, and in the test local is something different again), so we can't actually bump the patch.
as an approximation, given that it is just a dry-run and given that we always run it with 1000.0.1, what if we accept that for RCs we dry run on a schema that is 0.X.Y-RC.K?
@kelset @cortinico
…35296) Summary: Pull Request resolved: facebook#35296 This change adds some version checks and enforces that every version matches some specific format based on the build type we are trying to run. ## Changelog [General][Changed] - Improve version checks Reviewed By: cortinico Differential Revision: D41161756 fbshipit-source-id: 39c4c7ff72e0eba5708171ec11a8a857b3d5592e
91d0401
to
23265e1
Compare
This pull request was exported from Phabricator. Differential Revision: D41161756 |
PR build artifact for 23265e1 is ready. |
PR build artifact for 23265e1 is ready. |
…35296) Summary: Pull Request resolved: facebook#35296 This change adds some version checks and enforces that every version matches some specific format based on the build type we are trying to run. ## Changelog [General][Changed] - Improve version checks Reviewed By: cortinico Differential Revision: D41161756 fbshipit-source-id: fcae8101e57dbef79203881961b61d8663285fdf
680008e
to
7eb2c0a
Compare
This pull request was exported from Phabricator. Differential Revision: D41161756 |
…35296) Summary: Pull Request resolved: facebook#35296 This change adds some version checks and enforces that every version matches some specific format based on the build type we are trying to run. ## Changelog [General][Changed] - Improve version checks Reviewed By: cortinico Differential Revision: D41161756 fbshipit-source-id: 3a889967ab6a66357d68e380409fb061f831b670
7eb2c0a
to
a6880f8
Compare
PR build artifact for a6880f8 is ready. |
…35296) Summary: Pull Request resolved: facebook#35296 This change adds some version checks and enforces that every version matches some specific format based on the build type we are trying to run. ## Changelog [General][Changed] - Improve version checks Reviewed By: cortinico Differential Revision: D41161756 fbshipit-source-id: aba945afc14facad9fdf7a8cc6d14542c3bf03ee
This pull request was exported from Phabricator. Differential Revision: D41161756 |
a6880f8
to
0b3e187
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR build artifact for 0b3e187 is ready. |
PR build artifact for 0b3e187 is ready. |
Summary: Pull Request resolved: #35296 This change adds some version checks and enforces that every version matches some specific format based on the build type we are trying to run. ## Changelog [General][Changed] - Improve version checks Reviewed By: cortinico Differential Revision: D41161756 fbshipit-source-id: 4172195c5e031c1eaf7b33bb74f381c04e9adaf5
…test versioning (#35846) Summary: While working on 0.71 branch I encountered a problem in testing locally. Basically, I was getting hit by a silent error caused by recent work #35296 that didn't account for the shape of E2E local script for the release, `0.71.0-20230116-1649`. This scripts fixes both aspects: the error now gets thrown "better" and the logic accounts for the E2E shape. ## 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] - add logic for version scripts to account for local E2E test versioning Pull Request resolved: #35846 Test Plan: Tested via the other PR: #35847 Reviewed By: cortinico Differential Revision: D42543200 Pulled By: cipolleschi fbshipit-source-id: 727eb887fcbd183ec56d8a9b7e98241eaacb1d98
…35296) Summary: Pull Request resolved: facebook#35296 This change adds some version checks and enforces that every version matches some specific format based on the build type we are trying to run. ## Changelog [General][Changed] - Improve version checks Reviewed By: cortinico Differential Revision: D41161756 fbshipit-source-id: 4172195c5e031c1eaf7b33bb74f381c04e9adaf5
…test versioning (facebook#35846) Summary: While working on 0.71 branch I encountered a problem in testing locally. Basically, I was getting hit by a silent error caused by recent work facebook#35296 that didn't account for the shape of E2E local script for the release, `0.71.0-20230116-1649`. This scripts fixes both aspects: the error now gets thrown "better" and the logic accounts for the E2E shape. ## 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] - add logic for version scripts to account for local E2E test versioning Pull Request resolved: facebook#35846 Test Plan: Tested via the other PR: facebook#35847 Reviewed By: cortinico Differential Revision: D42543200 Pulled By: cipolleschi fbshipit-source-id: 727eb887fcbd183ec56d8a9b7e98241eaacb1d98
Summary:
This change adds some version checks and enforces that every version matches some specific format based on the build type we are trying to run.
Changelog
[General][Changed] - Improve version checks
Differential Revision: D41161756