Rename "wrangler_ssh" to "ssh" in Containers configuration#13567
Rename "wrangler_ssh" to "ssh" in Containers configuration#13567
Conversation
🦋 Changeset detectedLatest commit: 7b2c8d4 The changes in this PR will be included in the next version bump. 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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
|
✅ All changesets look good |
|
The review has been posted on PR #13567. Here's a summary of the findings: Two issues identified:
A concrete suggestion with both fixes was posted as a code suggestion on the PR. |
7d0bbbc to
c1e4acc
Compare
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
c1e4acc to
101080d
Compare
|
/bonk rereview |
There was a problem hiding this comment.
Re-reviewed after the update. The deprecation warning for wrangler_ssh has been added (lines 3339-3343) and is now consistent with the pattern used by the other deprecated container fields (instances, durable_objects, configuration). The backward-compat test at line 2052 verifies the warning is emitted.
The previous review also flagged the missing "both ssh and wrangler_ssh specified" error. Looking at the rest of the codebase, none of the other deprecated container fields (instances/max_instances, configuration/top-level fields) emit an error when both old and new forms are present — they just warn and move on. So the current behavior (deprecation warning + ssh wins) is consistent with the existing convention. I don't think adding a special-case error here is necessary.
Everything else looks good:
- Type definitions in
environment.tsare clean —sshis the primary field,wrangler_sshis@deprecatedand@hidden - The rewrite logic (
ssh→wrangler_sshfor the API) at lines 3403-3407 is correct - Validation uses the user-facing field name in error messages via
sshField - Port validation correctly guards the range check with
typeof sshPort === "number"(fixes a subtle bug from the original code whereundefined < 1would evaluate tofalseand silently pass) - Test coverage is solid: new field, backward compat alias with deprecation warning assertion, validation errors, and schema test
- Changeset is well-formed
|
Re-review posted on PR #13567 with an approval. The deprecation warning from the first review has been addressed, and the remaining suggestion (error when both |
"wrangler_ssh" is redundant since this option is set in the Wrangler config file. Rename it to just "ssh", while still supporting "wrangler_ssh" for backward compatibility.
101080d to
dfa4c8e
Compare
Looks like cloudchamber isn't listed as a codeowner for containers/ and cloudchamber/ tests? I wonder if this can be updated by us. It would be nice to get notifications about changes to those files. |
emily-shen
left a comment
There was a problem hiding this comment.
will the diffs on deploy still show 'wrangler_ssh'?
Yes, good catch. I just pushed a change to fix that so that it shows |
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
#13573 here you go |
"wrangler_ssh" is redundant since this option is set in the Wrangler config file. Rename it to just "ssh", while still supporting "wrangler_ssh" for backward compatibility.