-
Notifications
You must be signed in to change notification settings - Fork 757
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
Implement versions list
+ versions view
commands
#5208
Conversation
🦋 Changeset detectedLatest commit: efd3514 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
28c06f0
to
724d2d4
Compare
d909fc7
to
439ca2d
Compare
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/8262522410/npm-package-wrangler-5208 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5208/npm-package-wrangler-5208 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8262522410/npm-package-wrangler-5208 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8262522410/npm-package-create-cloudflare-5208 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8262522410/npm-package-cloudflare-kv-asset-handler-5208 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8262522410/npm-package-miniflare-5208 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8262522410/npm-package-cloudflare-pages-shared-5208 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8262522410/npm-package-cloudflare-vitest-pool-workers-5208 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5208 +/- ##
==========================================
+ Coverage 70.91% 70.98% +0.07%
==========================================
Files 301 304 +3
Lines 15831 15927 +96
Branches 4053 4080 +27
==========================================
+ Hits 11226 11306 +80
- Misses 4605 4621 +16
|
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.
Added some comments, but all non-blocking. Looks good! ✅ Assuming no changeset as this is still experimental, or are you planning to add one before merging?
`[APIError: A request to the Cloudflare API (/accounts/some-account-id/workers/scripts/test-name/versions/ffffffff-ffff-ffff-ffff-ffffffffffff) failed.]` | ||
); | ||
|
||
expect(normalizeOutput(std.out)).toMatchInlineSnapshot(` |
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.
Should this not be logged to stderr?
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.
Interesting. This is just the fetchResult call rejecting. Which should bubble up to the main() try..catch error handling. Does wrangler log errors to stdout??
@@ -0,0 +1,22 @@ | |||
import { logger } from "../logger"; | |||
|
|||
export default function renderLabelledValues( |
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.
A comment with example output from this function might be nice.
type UUID = string; | ||
type VersionId = UUID; | ||
type ApiVersion = { | ||
id: VersionId; | ||
number: number; | ||
metadata: { | ||
created_on: string; | ||
modified_on: string; | ||
source: "api" | string; | ||
author_id: string; | ||
author_email: string; | ||
}; | ||
annotations?: { | ||
"workers/triggered_by"?: "upload" | string; | ||
"workers/message"?: string; | ||
"workers/tag"?: string; | ||
}; | ||
// other properties not typed as not used | ||
}; |
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.
Do these types get extracted into some common file in a later PR? They seem to be duplicated.
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.
Yep
case "terraform": | ||
return "Terraform 🏗️"; | ||
default: | ||
return "Other"; |
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.
Should we include the actual value for Other
and Unknown
below?
return "Other"; | |
return `Other (${source})`; |
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 was thinking that too – will add
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 this in the next PR instead abc57e3
author_email: string; | ||
}; | ||
annotations?: Record<string, string> & { | ||
"workers/triggered_by"?: "upload" | string; |
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.
Should this have the other values, or just be string
?
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 extract it into another file in the next PR so will leave this for now
(I typed it from copy-pasting a api response so kept values for sanity knowing they'd widen into string anyway)
19dbe5a
to
96b38f7
Compare
f1b4772
to
a608e35
Compare
32b6d43
to
b681d4e
Compare
2f6d5e8
to
1310333
Compare
1310333
to
8c2292a
Compare
for versions view/list
8c2292a
to
efd3514
Compare
What this PR solves / how to test
This PR implements the
versions list
andversions view <version-id>
commands. They are very similar so a single PR seemed fine.Author has addressed the following