-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: use unique install cache folders for betas #20296
Conversation
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
guides/release-process.md
Outdated
![commit-bot-comment](../assets/cypress-bot-pre-release-comment.png) | ||
- Make sure the `linux-x64` binary and npm package are present at the commented locations. See [Before Publishing a New Version](#before-publishing-a-new-version). | ||
- Publish the `linux-x64` distribution to the npm registry straight from the URL: | ||
- Use the `create-stable-npm-package` script to create a stable NPM package from the pre-prod `cypress.tgz` URL: |
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.
Would it be possible to update move-binaries to handle this for us so there is one less manual step & potential release error?
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.
I think so. I was trying to be as deliberate as possible with making this its own step, so whoever is releasing is aware of what they're doing, but I guess it could be a source for errors. I'll look at this.
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.
@@ -29,6 +34,12 @@ function preparePackageForNpmRelease (json) { | |||
|
|||
_.extend(json, { | |||
version, | |||
buildInfo: { |
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.
late to the game, but feels like publishing the beta version to npm would remove the need for a lot of this logic
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.
I don't think publishing to NPM would really help avoid this complexity, unless we want to shove this complexity off to download.cypress.io
like we do for stable releases, something like this is necessary.
I would be down to publish to NPM regularly, like maybe a Nightly build as long as there have been new commits. We already release so frequently that anything less frequent than Nightly wouldn't be super useful.
cli/lib/tasks/install.js
Outdated
@@ -278,45 +202,32 @@ const start = (options = {}) => { | |||
logger.log() | |||
} | |||
|
|||
const pkgVersion = util.pkgVersion() | |||
const versionToInstall = getVersionOverride({ envVarVersion }) || pkgVersion |
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.
total nit, but for the sense of flow while reading, could we move this check above for readability?
const pkgVersion = util.pkgVersion()
const versionToInstall = getVersionOverride({ envVarVersion }) || pkgVersion
if (versionToInstall === '0') {
cli/lib/tasks/install.js
Outdated
logger.log( | ||
chalk.yellow(stripIndent` | ||
${logSymbols.warning} Warning: Forcing a binary version different than the default. | ||
if (versionToInstall !== pkgVersion) { |
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.
Will this alway log for binary version installs?
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.
Yup. The log message here isn't great, I was kinda just following what was here already, but I can improve it so that it indicates we're installing a pre-release and there's not a "different binary version"
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.
Log message updated, see snapshots. Also updated output for cypress info
.
|
||
```shell | ||
yarn move-binaries --sha <commit sha> --version <new target version> | ||
yarn prepare-release-artifacts --sha <commit sha> --version <new target version> |
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.
Since this run's on the releaser's local machine, should there be a requirements that this must run on a Mac or Linux machine to ensure line-endings are not corrupted?
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.
The script will always download the linux-x64
.tgz, but you raise a good point, possibly the stable: true
write will mess with the line endings. I'll test this out and see.
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.
In the process of testing this, I discovered that this script relies on behavior of tar
that is not present on Windows, so it fails anyways. Since only internal contributors use this script and we're all on MacOS and Linux, I went ahead and added a guard like you suggested bd305d1
Manually confirmed script works as expected on darwin. |
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
User facing changelog
Note: this commit is not attached to a specific release, but it is user-facing, so I guess it can go in the next release's changelog?
Additional details
package.json
buildInfo.stable !== true
stable: true
before publishing thisHow has the user experience changed?
cypress install
that clearly explains we're installing a pre-release.cypress info
has been updated to show if we are installing a pre-release.cypress version
displays 'pre-release' for the package version if applicable.cli/__snapshots__
for the CLI changes.PR Tasks
cypress-documentation
? docs: remove note about clearing cache when installing pre-releases cypress-documentation#4376type definitions
?cypress.schema.json
?