Add cloudchamber curl command #6126

IRCody · 2024-06-22T00:07:44Z

What this PR solves / how to test

Adds a cloudchamber curl command. Example usage:

wrangler cloudchamber curl /ssh-public-keys

CC-3877

Author has addressed the following

Tests
- TODO (before merge)
- Included
- Not necessary because:
E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required / Maybe required
- Not required because: Cloudchamber has no e2e tests
Changeset (Changeset guidelines)
- TODO (before merge)
- Included
- Not necessary because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Not necessary because: Cloudchamber is not documented externally

changeset-bot · 2024-06-22T00:07:48Z

🦋 Changeset detected

Latest commit: 7386e8f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
wrangler	Minor
@cloudflare/vitest-pool-workers	Patch

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

github-actions · 2024-06-22T08:26:29Z

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/10781169193/npm-package-wrangler-6126

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10781169193/npm-package-wrangler-6126 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10781169193/npm-package-create-cloudflare-6126 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10781169193/npm-package-cloudflare-kv-asset-handler-6126

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10781169193/npm-package-miniflare-6126

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10781169193/npm-package-cloudflare-pages-shared-6126

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10781169193/npm-package-cloudflare-vitest-pool-workers-6126

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10781169193/npm-package-cloudflare-workers-editor-shared-6126

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10781169193/npm-package-cloudflare-workers-shared-6126

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

wrangler@3.75.0 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240821.1
`workerd`	1.20240821.1	1.20240821.1
`workerd --version`	1.20240821.1	2024-08-21

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

IRCody · 2024-06-28T00:20:00Z

The test failures seem to be coming from something unrelated to this change:

  
   FAIL  src/__tests__/sentry.test.ts > sentry > interactive > should hit sentry after reportable error when permission provided
  Error: Snapshot `sentry > interactive > should hit sentry after reportable error when permission provided 3` mismatched```

threepointone · 2024-07-01T12:38:29Z

as you can tell, I've been poking at this PR. Looks like there's an actual failure on windows. I don't have a windows machine, so I'm debugging with CI. I'll keep poking at it.

threepointone · 2024-07-01T12:54:18Z

I tried debugging, but it's a bit confusing, so I'm going to move away from this, cc @petebacondarwin

IRCody · 2024-07-02T19:17:29Z

as you can tell, I've been poking at this PR. Looks like there's an actual failure on windows. I don't have a windows machine, so I'm debugging with CI. I'll keep poking at it.

I don't see the windows error in the current test failures. I also do not have a windows machine to test on. What errors were showing up there?

IRCody · 2024-07-08T17:25:14Z

Rebased to try to keep this pr up to date. Still unsure what the issues are in CI.

IRCody · 2024-07-12T21:14:03Z

as you can tell, I've been poking at this PR. Looks like there's an actual failure on windows. I don't have a windows machine, so I'm debugging with CI. I'll keep poking at it.

I don't see the windows error in the current test failures. I also do not have a windows machine to test on. What errors were showing up there?

Now that I'm in the org and can re-run the tests I can see this failure at least. Still investigating why it's happening and what would be different on windows specifically here as this passes on ubuntu/mac.

   FAIL  src/__tests__/cloudchamber/curl.test.ts > cloudchamber curl > should be able to use data flag
  Error: Snapshot `cloudchamber curl > should be able to use data flag 2` mismatched
  
  - Expected
  + Received
  
  - "{
  -     \"id\": \"1\",
  -     \"type\": \"default\",
  -     \"created_at\": \"123\",
  -     \"account_id\": \"123\",
  -     \"vcpu\": 4,
  -     \"memory\": \"400MB\",
  -     \"version\": 1,
  -     \"image\": \"hello\",
  -     \"location\": {
  -         \"name\": \"sfo06\",
  -         \"enabled\": true
  -     },
  -     \"network\": {
  -         \"ipv4\": \"1.1.1.1\"
  -     },
  -     \"placements_ref\": \"http://ref\",
  -     \"node_group\": \"metal\"
  - }"
  + "[object Object]"
  
   ❯ src/__tests__/cloudchamber/curl.test.ts:71:19
       69|   );
       70|   expect(std.err).toMatchInlineSnapshot(`""`);
       71|   expect(std.out).toMatchInlineSnapshot(`
         |                   ^
       72|    "{
       73|        \\"id\\": \\"1\\",

IRCody · 2024-07-12T22:55:17Z

The issue with the windows tests was differences in escaping. Fixed by using JSON.stringify so we get the correct escaping for whatever platform the tests are run on.

.changeset/new-dragons-bow.md

packages/wrangler/src/__tests__/cloudchamber/curl.test.ts

packages/wrangler/src/cloudchamber/curl.ts

packages/wrangler/src/cloudchamber/index.ts

IRCody · 2024-07-26T17:36:15Z

@petebacondarwin: I tried to address all your points. Appreciate if you have time to take another look.

IRCody · 2024-08-02T17:29:03Z

@petebacondarwin: Thought I'd ping again to see if you have time to take another look.

petebacondarwin · 2024-08-07T12:50:58Z

packages/wrangler/src/cloudchamber/curl.ts

+		silent?: boolean;
+		verbose?: boolean;


Do these have meaning over and above the WRANGLER_LOG env var that can control what logger methods actually write to the console?
Is there a reason you are not using the logger?

I thought that users might want to have different output preferences for curl specifically than most other commands but I agree it's a little inconsistent and potentially confusing.

Do you think it would make sense to change this to if the user requests a json response we print out in the current way but otherwise we use the logger and respect WRANGLER_LOG?

I think we'd need to be careful with the output here. Is it expected that this command's output will be piped to other processes? If so, would changes to the output be considered a breaking change?

Since we've provided a --json flag, we should guide scripting users to that terse output which will reflect the backcompat of the api being hit. Leaving us more free to iterate on the human-readable output without fear of breaking anyone's workflow.

I don't see a problem with the --verbose flag, since WRANGLER_LOG or --log-level don't have a concept of verbosity, only debug logging. My only issue is if it encourages users to script against stdout, preventing us from iterating on this command.

IRCody · 2024-08-19T17:06:36Z

@petebacondarwin: Do the current changes look ok?

petebacondarwin

LGTM - might be worth getting second set of eyes from @RamIdeas, since he is interested in how we do CLI input and output and JSON etc.

.changeset/new-dragons-bow.md

RamIdeas · 2024-08-20T13:09:50Z

packages/wrangler/src/cloudchamber/curl.ts

+			logRaw(
+				text
+					.split("\n")
+					.map((line) => `${yellow(`\t`)} ${brandColor(line)}`)


Is there a need for \t to be yellow?

I'm also not sure what this .split/.map/.join is doing, since JSON.stringify(..., 4) already indents the output

The split/map/join is just adding the color.

Is there a need for \t to be yellow?

The \t doesn't need to be there at all actually, thanks!

packages/wrangler/src/cloudchamber/curl.ts

RamIdeas · 2024-08-20T13:49:41Z

packages/wrangler/src/cloudchamber/curl.ts

+		silent?: boolean;
+		verbose?: boolean;


I think we'd need to be careful with the output here. Is it expected that this command's output will be piped to other processes? If so, would changes to the output be considered a breaking change?

Since we've provided a --json flag, we should guide scripting users to that terse output which will reflect the backcompat of the api being hit. Leaving us more free to iterate on the human-readable output without fear of breaking anyone's workflow.

I don't see a problem with the --verbose flag, since WRANGLER_LOG or --log-level don't have a concept of verbosity, only debug logging. My only issue is if it encourages users to script against stdout, preventing us from iterating on this command.

IRCody · 2024-08-22T01:05:10Z

Thanks for taking a look at this @RamIdeas!

I think we'd need to be careful with the output here. Is it expected that this command's output will be piped to other processes? If so, would changes to the output be considered a breaking change?

My expectation would be that output from this command that was being piped would use the json flag.

Since we've provided a --json flag, we should guide scripting users to that terse output which will reflect the backcompat of the api being hit. Leaving us more free to iterate on the human-readable output without fear of breaking anyone's workflow.

I don't see a problem with the --verbose flag, since WRANGLER_LOG or --log-level don't have a concept of verbosity, only debug logging. My only issue is if it encourages users to script against stdout, preventing us from iterating on this command.

Do you think it would make sense to make json output the default as a way to steer people towards it and instead have a --human flag or something for human readable text?

Switch to using formatLabelledValues from utils. Remove some unneeded conditions in an else block.

RamIdeas · 2024-08-23T09:14:32Z

My expectation would be that output from this command that was being piped would use the json flag.

Do you think it would make sense to make json output the default as a way to steer people towards it and instead have a --human flag or something for human readable text?

I think we should remain consistent with a --json flag that defaults to false.

I think we should call it out in the --json flag description that it is should be used if the output is being piped. Are there docs for this command? Maybe also call it out there.

IRCody · 2024-09-09T18:59:57Z

I think we should remain consistent with a --json flag that defaults to false.

I think we should call it out in the --json flag description that it is should be used if the output is being piped. Are there docs for this command? Maybe also call it out there.

Sorry for the delay here. I added some text to the json flag description about using it for consistent, machine readable output. Does this match what you were looking for?

IRCody · 2024-09-09T21:24:44Z

@RamIdeas: Sorry I had to make a small additional change to make the type check happy.

IRCody requested review from a team as code owners June 22, 2024 00:07

IRCody force-pushed the cloudchamber-curl branch from b36cb7f to 01aa49c Compare June 24, 2024 18:50

ichernetsky-cf approved these changes Jun 24, 2024

View reviewed changes

gabivlj approved these changes Jun 24, 2024

View reviewed changes

threepointone force-pushed the cloudchamber-curl branch 5 times, most recently from c4745ed to c93e8aa Compare July 1, 2024 12:32

threepointone force-pushed the cloudchamber-curl branch from c93e8aa to 26f906f Compare July 1, 2024 12:53

IRCody force-pushed the cloudchamber-curl branch from 26f906f to 45c12e8 Compare July 8, 2024 17:23

IRCody force-pushed the cloudchamber-curl branch from 45c12e8 to 7c8146e Compare July 12, 2024 20:41

IRCody force-pushed the cloudchamber-curl branch 3 times, most recently from 4a35958 to 8e24836 Compare July 12, 2024 22:31

petebacondarwin requested changes Jul 15, 2024

View reviewed changes

IRCody force-pushed the cloudchamber-curl branch from 8e24836 to bfc5016 Compare July 22, 2024 23:58

IRCody requested a review from petebacondarwin July 23, 2024 15:01

Add cloudchamber curl command

efa2c8d

IRCody force-pushed the cloudchamber-curl branch from bfc5016 to efa2c8d Compare July 26, 2024 19:24

IRCody added the awaiting Cloudflare response Awaiting response from workers-sdk maintainer team label Jul 29, 2024

petebacondarwin approved these changes Aug 7, 2024

View reviewed changes

Switch from console.log to logRaw

22a6a45

petebacondarwin approved these changes Aug 20, 2024

View reviewed changes

RamIdeas requested changes Aug 20, 2024

View reviewed changes

Cleanup some output related code

b7df331

Switch to using formatLabelledValues from utils. Remove some unneeded conditions in an else block.

RamIdeas approved these changes Sep 9, 2024

View reviewed changes

Add extra context to json flag description

7386e8f

IRCody force-pushed the cloudchamber-curl branch from 2d126cd to 7386e8f Compare September 9, 2024 20:58

RamIdeas merged commit 18c105b into cloudflare:main Sep 10, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cloudchamber curl command #6126

Add cloudchamber curl command #6126

IRCody commented Jun 22, 2024 •

edited by penalosa

Loading

changeset-bot bot commented Jun 22, 2024 •

edited

Loading

github-actions bot commented Jun 22, 2024 •

edited

Loading

IRCody commented Jun 28, 2024

threepointone commented Jul 1, 2024

threepointone commented Jul 1, 2024

IRCody commented Jul 2, 2024 •

edited

Loading

IRCody commented Jul 8, 2024

IRCody commented Jul 12, 2024

IRCody commented Jul 12, 2024

IRCody commented Jul 26, 2024

IRCody commented Aug 2, 2024

petebacondarwin Aug 7, 2024

IRCody Aug 12, 2024

RamIdeas Aug 20, 2024

IRCody commented Aug 19, 2024

petebacondarwin left a comment

RamIdeas Aug 20, 2024

RamIdeas Aug 20, 2024

IRCody Aug 22, 2024

RamIdeas Aug 20, 2024

IRCody commented Aug 22, 2024 •

edited

Loading

RamIdeas commented Aug 23, 2024

IRCody commented Sep 9, 2024

IRCody commented Sep 9, 2024

Add cloudchamber curl command #6126

Add cloudchamber curl command #6126

Conversation

IRCody commented Jun 22, 2024 • edited by penalosa Loading

What this PR solves / how to test

Author has addressed the following

changeset-bot bot commented Jun 22, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Jun 22, 2024 • edited Loading

IRCody commented Jun 28, 2024

threepointone commented Jul 1, 2024

threepointone commented Jul 1, 2024

IRCody commented Jul 2, 2024 • edited Loading

IRCody commented Jul 8, 2024

IRCody commented Jul 12, 2024

IRCody commented Jul 12, 2024

IRCody commented Jul 26, 2024

IRCody commented Aug 2, 2024

petebacondarwin Aug 7, 2024

Choose a reason for hiding this comment

IRCody Aug 12, 2024

Choose a reason for hiding this comment

RamIdeas Aug 20, 2024

Choose a reason for hiding this comment

IRCody commented Aug 19, 2024

petebacondarwin left a comment

Choose a reason for hiding this comment

RamIdeas Aug 20, 2024

Choose a reason for hiding this comment

RamIdeas Aug 20, 2024

Choose a reason for hiding this comment

IRCody Aug 22, 2024

Choose a reason for hiding this comment

RamIdeas Aug 20, 2024

Choose a reason for hiding this comment

IRCody commented Aug 22, 2024 • edited Loading

RamIdeas commented Aug 23, 2024

IRCody commented Sep 9, 2024

IRCody commented Sep 9, 2024

IRCody commented Jun 22, 2024 •

edited by penalosa

Loading

changeset-bot bot commented Jun 22, 2024 •

edited

Loading

github-actions bot commented Jun 22, 2024 •

edited

Loading

IRCody commented Jul 2, 2024 •

edited

Loading

IRCody commented Aug 22, 2024 •

edited

Loading