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

feat: Custom https cert #4475

Merged
merged 7 commits into from
Feb 16, 2024
Merged

Conversation

paulrostorp
Copy link
Contributor

@paulrostorp paulrostorp commented Nov 21, 2023

Fixes #2118

What this PR solves / how to test:
Add flags --https-key-path and --https-cert-path to wrangler pages dev command

Author has addressed the following:

Copy link

changeset-bot bot commented Nov 21, 2023

⚠️ No Changeset found

Latest commit: bbc7f0a

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

@lrapoport-cf
Copy link
Contributor

blocked for now (see #2118 (comment))

@petebacondarwin
Copy link
Contributor

I know this is blocked for the moment, but I would like to propose a simpler implementation, for when we are in a position to land this. Instead of using command line arguments, which need to be threaded down through the various calls. How about we just have a pair of environment variables, e.g. CLOUDFLARE_LOCAL_HTTPS_KEY_PATH and CLOUDFLARE_LOCAL_HTTPS_CERT_PATH.
Then the implementation would simply check these environment variables in the getHttpsOptions() function before attempting to generate these files.

Is there a reason (e.g. prior art) for doing this via command line arguments that I am missing?

@paulrostorp
Copy link
Contributor Author

@petebacondarwin I did consider that as it was a much simpler implementation, but considering this CLI aims to support configuration both through flags and a config file (wrangler.toml), I thought it best not to break that pattern.

@petebacondarwin
Copy link
Contributor

@petebacondarwin I did consider that as it was a much simpler implementation, but considering this CLI aims to support configuration both through flags and a config file (wrangler.toml), I thought it best not to break that pattern.

Ah yes and looking through the original issue it seems that there is value in being able to set different certs for each wrangler instance, which is more awkward using env vars - #2118 (comment)

@1000hz 1000hz removed the blocked-CO label Jan 22, 2024
@lrapoport-cf lrapoport-cf added awaiting Cloudflare response Awaiting response from workers-sdk maintainer team and removed awaiting Cloudflare response Awaiting response from workers-sdk maintainer team labels Jan 30, 2024
@petebacondarwin petebacondarwin added enhancement New feature or request and removed pages Relating to Pages labels Feb 12, 2024
Copy link
Contributor

github-actions bot commented Feb 12, 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/7932008257/npm-package-wrangler-4475

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

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

Or you can use npx with this latest build directly:

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

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


wrangler@3.28.2 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240129.2
workerd 1.20240129.0 1.20240129.0
workerd --version 1.20240129.0 2024-01-29

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

Copy link

codecov bot commented Feb 12, 2024

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (55ea072) 70.44% compared to head (bbc7f0a) 70.40%.
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4475      +/-   ##
==========================================
- Coverage   70.44%   70.40%   -0.05%     
==========================================
  Files         297      297              
  Lines       15431    15448      +17     
  Branches     3951     3960       +9     
==========================================
+ Hits        10871    10876       +5     
- Misses       4560     4572      +12     
Files Coverage Δ
packages/wrangler/src/api/dev.ts 83.33% <100.00%> (+0.47%) ⬆️
packages/wrangler/src/api/startDevWorker/types.ts 100.00% <ø> (ø)
packages/wrangler/src/dev.tsx 80.30% <ø> (ø)
packages/wrangler/src/dev/local.tsx 27.50% <ø> (ø)
packages/wrangler/src/dev/remote.tsx 7.92% <ø> (ø)
packages/wrangler/src/dev/start-server.ts 78.19% <ø> (ø)
packages/wrangler/src/pages/dev.ts 16.21% <ø> (ø)
packages/wrangler/src/dev/miniflare.ts 63.55% <0.00%> (ø)
...wrangler/src/api/startDevWorker/ProxyController.ts 61.97% <0.00%> (+0.83%) ⬆️
packages/wrangler/src/dev/proxy.ts 4.11% <0.00%> (ø)
... and 1 more

... and 1 file with indirect coverage changes

@petebacondarwin
Copy link
Contributor

I rebased and fixed up this branch. Will do a proper review later this week.

@petebacondarwin petebacondarwin added the e2e Run e2e tests on a PR label Feb 13, 2024
@petebacondarwin petebacondarwin removed the e2e Run e2e tests on a PR label Feb 16, 2024
@petebacondarwin petebacondarwin merged commit 86d94ff into cloudflare:main Feb 16, 2024
17 checks passed
@Aphanite
Copy link

Aphanite commented Mar 2, 2024

Thanks a lot for this and for including it in the latest release! However, when I searched for the first version to support it, I found no mention of it in the recent releases.

@petebacondarwin
Copy link
Contributor

Gosh! You are right. That is my mistake for landing this without a changeset. It was first available in 3.28.3. I'll update the changelog accordingly.

@petebacondarwin
Copy link
Contributor

#5147

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add ability to provide certs for https local dev server
6 participants