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

wrangler: Try to URL decode hyperdrive connection string components before sending them to API #5258

Conversation

OilyLime
Copy link
Contributor

@OilyLime OilyLime commented Mar 14, 2024

What this PR solves / how to test

Fixes SQC-153
LDW CR-851053

Author has addressed the following

  • Tests
    • TODO (before merge)
    • Included
    • Not necessary because:
  • Changeset (Changeset guidelines)
    • TODO (before merge)
    • Included
    • Not necessary because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Not necessary because: Does not change the Hyperdrive command or its options.

@OilyLime OilyLime requested a review from a team as a code owner March 14, 2024 18:57
Copy link

changeset-bot bot commented Mar 14, 2024

🦋 Changeset detected

Latest commit: 649aba2

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

This PR includes changesets to release 4 packages
Name Type
miniflare Minor
wrangler Minor
@cloudflare/pages-shared Patch
@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

Copy link
Contributor

github-actions bot commented Mar 14, 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/8456923506/npm-package-wrangler-5258

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8456923506/npm-package-wrangler-5258 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8456923506/npm-package-create-cloudflare-5258 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8456923506/npm-package-cloudflare-kv-asset-handler-5258
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8456923506/npm-package-miniflare-5258
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8456923506/npm-package-cloudflare-pages-shared-5258
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8456923506/npm-package-cloudflare-vitest-pool-workers-5258

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


wrangler@3.39.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240320.0
workerd 1.20240320.1 1.20240320.1
workerd --version 1.20240320.1 2024-03-20

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

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.27%. Comparing base (0c535e7) to head (649aba2).
Report is 65 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5258      +/-   ##
==========================================
+ Coverage   71.46%   72.27%   +0.81%     
==========================================
  Files         309      322      +13     
  Lines       16097    16666     +569     
  Branches     4111     4258     +147     
==========================================
+ Hits        11504    12046     +542     
- Misses       4593     4620      +27     
Files Coverage Δ
packages/wrangler/src/hyperdrive/create.ts 78.12% <ø> (ø)
packages/wrangler/src/hyperdrive/update.ts 100.00% <100.00%> (+3.12%) ⬆️

... and 58 files with indirect coverage changes

@mrbbot mrbbot added the awaiting reporter response Needs clarification or followup from OP label Mar 25, 2024
…efore sending them to API

Also do the same in miniflare plugin
@OilyLime OilyLime force-pushed the oilylime/urldecode-hyperdrive-passwords branch from 6b04ceb to 5f64394 Compare March 25, 2024 20:15
Copy link
Contributor

@mrbbot mrbbot left a comment

Choose a reason for hiding this comment

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

Looks good! If this can wait until after the LDW, we can merge this then. Otherwise, you'll need a CR for this change.

@mrbbot
Copy link
Contributor

mrbbot commented Mar 26, 2024

It looks like you'll need to run pnpm run prettify too to format your code.

@OilyLime
Copy link
Contributor Author

It looks like you'll need to run pnpm run prettify too to format your code.

Ran it but no files were changed 🤷

@penalosa
Copy link
Contributor

Ran it but no files were changed 🤷

It looks like the specific file with issues is packages/wrangler/src/hyperdrive/update.ts—could you try formatting that directly?

@petebacondarwin petebacondarwin added ldw-exempt PRs marked with this label can be included in releases during LDW. and removed awaiting reporter response Needs clarification or followup from OP labels Mar 27, 2024
@petebacondarwin petebacondarwin merged commit fbdca7d into cloudflare:main Mar 27, 2024
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ldw-exempt PRs marked with this label can be included in releases during LDW.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants