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

Migrate prepare-release-from-ci to new workflow #20581

Merged
merged 2 commits into from
Jan 14, 2021

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Jan 13, 2021

I added a --releaseChannel (-r) argument to the script. You must choose either "stable" or "experimental", because every build job now includes both channels.

The prepare-release-from-npm script is unchanged since those releases are downloaded from npm, not CI.

(As a side note, I think we should start preparing semver releases using the prepare-release-from-ci script, too, and get rid of prepare-release-from-npm. I think downloading from npm was a neat idea originally, but because we already run npm pack before storing the artifacts in CI, there's really not much additional safety; the only safeguard it adds is the requirement that a "next" release must have already been published.)

Test plan

Prepare a stable release:

./scripts/release/prepare-release-from-ci.js --skipTests -r stable

Prepare an experimental release:

./scripts/release/prepare-release-from-ci.js --skipTests -r experimental

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jan 13, 2021
const compressedPackages = readdirSync(join(cwd, 'build/node_modules/'));
for (let i = 0; i < compressedPackages.length; i++) {
await exec(
`tar zxvf ./build/node_modules/${compressedPackages[i]} -C ./build/node_modules/`,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When this was originally added, we didn't store the entire build directory as a CI artifact. Just the npm packages.

The reason we stored each module as an individual tarball was because we ran npm pack to run any npm pre-processing, like filtering files out based on the package.files array. The build script already does this, though, so we can skip the extra step.

@sizebot
Copy link

sizebot commented Jan 13, 2021

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 6ea9ed6

@sizebot
Copy link

sizebot commented Jan 13, 2021

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 6ea9ed6

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 13, 2021

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 6ea9ed6:

Sandbox Source
React Configuration

@bvaughn
Copy link
Contributor

bvaughn commented Jan 14, 2021

(As a side note, I think we should start preparing semver releases using the prepare-release-from-ci script, too, and get rid of prepare-release-from-npm. I think downloading from npm was a neat idea originally, but because we already run npm pack before storing the artifacts in CI, there's really not much additional safety; the only safeguard it adds is the requirement that a "next" release must have already been published.)

What's the main motivation for this change? Seems like that's a nice safeguard, though it's admittedly not totally necessary. What additional benefit do we get from publishing from CI?

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

👍

const channel = params.releaseChannel;
if (channel !== 'experimental' && channel !== 'stable') {
console.error(
`Invalid release channel: "${channel}". Must be "stable" or "experimental".`
Copy link
Contributor

Choose a reason for hiding this comment

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

May help clarify a tad to call out the param name too?

Suggested change
`Invalid release channel: "${channel}". Must be "stable" or "experimental".`
`Invalid release channel (-r) "${channel}". Must be "stable" or "experimental".`

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better still to follow precedent here and add this validation into parse-params.js e.g.

params.tags.forEach(tag => {
switch (tag) {
case 'latest':
case 'next':
case 'experimental':
case 'untagged':
break;
default:
console.error('Unknown tag: "' + params.tag + '"');
process.exit(1);
break;
}

@acdlite
Copy link
Collaborator Author

acdlite commented Jan 14, 2021

What's the main motivation for [getting rid of the prepare from npm script]? Seems like that's a nice safeguard, though it's admittedly not totally necessary. What additional benefit do we get from publishing from CI?

Not much other than fewer things to maintain, or accidentally subtly break.

I added a `--releaseChannel (-r)` argument to script. You must choose
either "stable" or "experimental", because every build job now includes
both channels.

The prepare-release-from-npm script is unchanged since those releases
are downloaded from npm, nt CI.

(As a side note, I think we should start preparing semver releases using
the prepare-release-from-ci script, too, and get rid of
prepare-release-from-npm. I think that was a neat idea originally but
because we already run `npm pack` before storing the artifacts in CI,
there's really not much additional safety; the only safeguard it adds is
the requirement that a "next" release must have already been published.)
@acdlite acdlite merged commit 98313aa into facebook:master Jan 14, 2021
@rickhanlonii
Copy link
Member

Is the goal for this to eventually support www-modern and www-classic too?

// Download and extract artifact
await exec(`rm -rf ./build2`, {cwd});
await exec(
`curl -L $(fwdproxy-config curl) ${buildArtifacts.url} | tar -xvz`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @acdlite I'm trying to execute this script on a Windows machine and it looks like it assumes Unix heavily. I'm trying to rewrite this in a more xplat fashion, but I can't figure out what fwdproxy-config curl is or returns. If you could point me to the package that enables this command or something, that'd be great. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@acdlite friendly ping

koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
* Migrate prepare-release-from-ci to new workflow

I added a `--releaseChannel (-r)` argument to script. You must choose
either "stable" or "experimental", because every build job now includes
both channels.

The prepare-release-from-npm script is unchanged since those releases
are downloaded from npm, nt CI.

(As a side note, I think we should start preparing semver releases using
the prepare-release-from-ci script, too, and get rid of
prepare-release-from-npm. I think that was a neat idea originally but
because we already run `npm pack` before storing the artifacts in CI,
there's really not much additional safety; the only safeguard it adds is
the requirement that a "next" release must have already been published.)

* Move validation to parse-params module
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants