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

ci: easy pre-releases for new packages #4757

Merged
merged 8 commits into from
Jan 19, 2024
Merged

Conversation

mrbbot
Copy link
Contributor

@mrbbot mrbbot commented Jan 16, 2024

What this PR solves / how to test:

I'm planning to put a PR up for Miniflare 3's new Vitest unit-testing environment soon. This is going to be a separate @cloudflare/vitest-pool-workers package. While the PR is being reviewed, it would be nice to have pre-releases of the package available.

Currently, we have two pre-release workflows, one that's triggered on pull requests to build pre-releases and publish them as artifacts, and then another that's triggered from main when the first completes to write pre-release URLs to a sticky comment. Both of these workflows must be updated to add a new package to the pre-release registry.

As the 2nd workflow is triggered from main, a separate PR to add @cloudflare/vitest-pool-workers to the comment would be needed, before the PR implementing the package could be put up. When packages have dependencies on other pre-released packages, these dependencies must be hardcoded in the workflow too.

This change allows any package to be included in the pre-release registry by adding "workers-sdk": { "prerelease": true } to its package.json. Any dependencies that are also marked as pre-releasable are automatically updated to point to the correct pre-release URL.

Author has addressed the following:

  • Tests
    • Included
    • Not necessary because: changing GitHub actions workflows
  • Changeset (Changeset guidelines)
    • Included
    • Not necessary because: no user facing changes, just changing internal workflows
  • Associated docs
    • Issue(s)/PR(s):
    • Not necessary because: no user facing changes, just changing internal workflows

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

@mrbbot mrbbot requested review from a team as code owners January 16, 2024 15:24
Copy link

changeset-bot bot commented Jan 16, 2024

⚠️ No Changeset found

Latest commit: 565b549

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (42ad768) 75.66% compared to head (565b549) 71.83%.
Report is 18 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4757      +/-   ##
==========================================
- Coverage   75.66%   71.83%   -3.84%     
==========================================
  Files         243      285      +42     
  Lines       13226    14761    +1535     
  Branches     3396     3719     +323     
==========================================
+ Hits        10008    10603     +595     
- Misses       3218     4158     +940     

see 60 files with indirect coverage changes

Copy link
Member

@GregBrimble GregBrimble left a comment

Choose a reason for hiding this comment

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

Nice improvement!

Expose ACTIONS_RUNTIME_TOKEN and ACTIONS_RESULTS_URL variables
Switch to `workers-sdk` object in `package.json`
Copy link
Contributor

github-actions bot commented Jan 17, 2024

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7584753028/npm-package-wrangler-4757

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7584753028/npm-package-wrangler-4757

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7584753028/npm-package-wrangler-4757 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7584753028/npm-package-miniflare-4757
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7584753028/npm-package-cloudflare-pages-shared-4757
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7584753028/npm-package-create-cloudflare-4757 --no-auto-update

Note that these links will no longer work once the GitHub Actions artifact expires.


wrangler@3.23.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20231218.2
workerd 1.20231218.0 1.20231218.0
workerd --version 1.20231218.0 2023-12-18

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

assert.strictEqual(name, "wrangler");
const url = getPrereleaseArtifactUrl(name);
const prUrl = getPrereleasePRArtifactUrl(name);
return `A wrangler prerelease is available for testing. You can install this latest build in your project with:
Copy link
Member

Choose a reason for hiding this comment

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

I am not a big fan of the markdown being all included here like so (and all the backticks escapes)

I am fine with it, but hopefully we can come up with something more convenient at some point 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Also I've disliked the prerelease comment for a while and I did try to update it 😅

It's not something really related to this PR, but yeah I'd love it if we could improve the comment/report as a whole, I just figured I'd mention it here 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair. Could consider extracting the comment into a template file and using something like Handlebars? Although, given we don't modify the template that much, it might not be worth the additional complexity.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I was thinking of something like handlebars as well 🙂

But it was just a thought, I actually don't think we should overthink this in this PR and increase it's scope, I'm 100% fine with the current implementation, I just wanted to mention that it'd be nice maybe later at some point to improve things 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Captured in #4790 🙂

@dario-piotrowicz
Copy link
Member

@mrbbot sorry it is a bit unrelated... 😅

But in the runtime versions we sometimes get an extra |, as you can see in this PRs prerelease comment
#4757 (comment):
Screenshot 2024-01-18 at 18 05 03

Since you're already touching the prerelease could you also amend that? 🙂
Once I looked into this and I remember that the issue was an extra \n that we were getting and this change fixed it: https://github.com/cloudflare/workers-sdk/pull/4090/files#r1457812880

Copy link
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

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

Looks great to me! 🤩

I just left a few very very minor nitpicks 😅

Remove extraneous `|` in pre-release comment
@mrbbot
Copy link
Contributor Author

mrbbot commented Jan 18, 2024

@dario-piotrowicz Fixed that | issue in 6f9e2be

.github/prereleases/0-packages.mjs Outdated Show resolved Hide resolved
.github/prereleases/0-packages.mjs Show resolved Hide resolved
.github/prereleases/1-versions.mjs Outdated Show resolved Hide resolved
.github/prereleases/3-comment.mjs Show resolved Hide resolved

/** @returns {~Package[]} */
export function getPackagesForPrerelease() {
return getPackages().filter((pkg) => pkg.json["workers-sdk"]?.prerelease);
Copy link
Member

Choose a reason for hiding this comment

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

Just a quick idea, It doesn't need to be implemented in this PR but I wanted to mention it 🙂

This function gets all the packages that have the prerelease field regardless on whether they have been modified in the PR or not (at least that's my understanding)

Could we improve this by checking using git if files within the package have changes and if not, not generate a prerelease for it at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good idea, I've captured it along with other suggestions in #4790 🙂

Simplify validation/extraction of `githubPullRequestNumber`
Use `forEach` instead of `for of`
Explain why we need custom action for extracting action variables
@mrbbot mrbbot merged commit f860aba into main Jan 19, 2024
22 checks passed
@mrbbot mrbbot deleted the bcoll/generic-prereleases branch January 19, 2024 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants