Fix release script --commit param #20720
Conversation
PR facebook#20717 accidentally broke the `--commit` parameter because the script errors if you provide both a `--build` and a `--commit`. I solved by removing the validation error. When there's a conflict, it will choose the --`build`. (Although maybe we should `--build` entirely and always uses `--commit`. I think `--commit` is a sufficient replacement.)
facebook-github-bot
added
CLA Signed
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 65b3132:
|
|
How did #20717 break the That PR only affected the Edit Oh, it broke it if you pasted in the reproducible run with the |
| process.exit(1); | ||
| } | ||
| // TODO: Should we just remove the `build` param? Seems like `commit` is a | ||
| // sufficient replacement. |
There was a problem hiding this comment.
Looks like the release script README is in a broken state at the moment. I'll take a pass at that too with an update PR.
| '--build', | ||
| await getLatestMasterBuildNumber(true) | ||
| ); | ||
| addDefaultParamValue(null, '--commit', 'master'); |
There was a problem hiding this comment.
This is a simpler solution but as it stands after this PR, the Firefox DevTools release process will be broken (because their testers need to have a reproducible from source and scripts/release/download-experimental-build.js --commit=master isn't a valid way to repro a specific build). I'll propose a small change with a follow up PR though!
There was a problem hiding this comment.
Hm do we not include the build-info.json with CI builds anymore? I was hoping we could read from it instead but I don't see it in the build2 artifacts.
There was a problem hiding this comment.
Ooh now I see this:
But is there a reason why the new build script doesn't/can't output it? We're in a weird space now where our scripts both validate and require that build-info.json is in the package files array and we no longer publish it 😁
I guess I can always parse it out of the build number?
There was a problem hiding this comment.
But is there a reason why the new build script doesn't/can't output it?
Figured it was worth seeing if it's actually necessary anymore. Might be nice to stop requiring every package to publish that file to npm, if we can.
I believe the last remaining reason we need it is because the prepare-release-from-npm needs it. But maybe we can instead pull the build artifacts from CI, like we do with prereleases.
There was a problem hiding this comment.
I guess, but at the same time I don't see much of a downside of publishing it. It adds a little complexity in one place (but we already have it in place and well "tested") in exchange for avoiding it in another (having to pull a duplicate artifact from CI somehow– which seems difficult to do if we don't have build-info)
This was ported to the new workflow by facebook#20720
This was ported to the new workflow by #20720
PR facebook#20717 accidentally broke the `--commit` parameter because the script errors if you provide both a `--build` and a `--commit`. I solved by removing the validation error. When there's a conflict, it will choose the --`build`. (Although maybe we should `--build` entirely and always uses `--commit`. I think `--commit` is a sufficient replacement.)
This was ported to the new workflow by facebook#20720
PR #20717 accidentally broke the
--commitparameter because the script errors if you provide both a--buildand a--commit.I solved by removing the validation error. When there's a conflict, it will choose the
--build.(Although maybe we should remove
--buildand always use--commit. I think--commitis a sufficient replacement.)