feat(scripts): manage custom hostnames for Edge Scripts#89
Conversation
|
@codex review |
🦋 Changeset detectedLatest commit: 52c2dca The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
| * Defaults to the script's only linked pull zone. When several exist, | ||
| * requires `--pull-zone` to disambiguate. | ||
| */ | ||
| function resolvePullZoneId(script: EdgeScript, flag?: number): number { |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1368548a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await client.GET("/pullzone/loadFreeCertificate", { | ||
| params: { query: { hostname } }, | ||
| }); |
There was a problem hiding this comment.
When scripts hostnames ssl <hostname> --no-force-ssl runs, this account-wide /pullzone/loadFreeCertificate call is the only API request made; it is keyed only by hostname, not by the resolved pullZoneId. If the account has the hostname on a different pull zone, the command can issue/claim SSL for that other resource while reporting success for the selected script pull zone. Fetch the target pull zone's hostnames first, as remove does, or otherwise verify the hostname belongs to pullZoneId before calling this endpoint.
Useful? React with 👍 / 👎.
| if (args.output === "json") { | ||
| logger.log(JSON.stringify(hostnames, null, 2)); |
There was a problem hiding this comment.
When users run scripts hostnames list --output json, this serializes the raw HostnameModel objects returned by GET /pullzone/{id}; that model includes Certificate and CertificateKey, so any returned private-key material can be written to terminals or CI logs. scripts show --output json adds the same raw hostnames as well, so map hostnames to the safe fields used by the table output before JSON serialization.
Useful? React with 👍 / 👎.
| } catch (err) { | ||
| sslError = err instanceof Error ? err.message : String(err); | ||
| } |
There was a problem hiding this comment.
When scripts hostnames add <host> --ssl is used and enableSsl() fails (for example, DNS is not pointed correctly), this catch records the error but the command still exits successfully after printing Added ..., and JSON output reports success unless callers inspect sslError. Automation that requests --ssl can therefore continue even though the certificate/Force SSL step did not happen; the ssl subcommand already treats the same failure as a command error.
Useful? React with 👍 / 👎.
|
@greptile-apps review |
Greptile SummaryThis PR adds a
Confidence Score: 5/5Safe to merge — the new hostname commands are well-scoped, the factory pattern is clean, and the previously flagged silent-swallow issues have been addressed with debug logging. All changed paths follow established codebase conventions, errors are handled consistently, and private-key material is correctly stripped before any JSON serialization. No data-loss or incorrect-state paths were found in the changed code. commands.ts — the process.exit(1) call in the JSON-mode SSL-failure path and the always-generic DNS-propagation hint are worth a second look before the factory is reused for apps. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant ComputeAPI as Compute API
participant CoreAPI as Core API
Note over CLI,CoreAPI: scripts domains add domain --ssl
User->>CLI: bunny scripts domains add shop.example.com --ssl
CLI->>ComputeAPI: "GET /compute/script/{id}"
ComputeAPI-->>CLI: EdgeScriptModel (LinkedPullZones)
CLI->>CoreAPI: "POST /pullzone/{id}/addHostname"
CoreAPI-->>CLI: 200 OK
CLI->>CoreAPI: "GET /pullzone/{id} (fetch hostnames list)"
CoreAPI-->>CLI: Hostname[]
CLI->>CoreAPI: "GET /pullzone/loadFreeCertificate?hostname=..."
CoreAPI-->>CLI: 200 OK
CLI->>CoreAPI: "POST /pullzone/{id}/setForceSSL"
CoreAPI-->>CLI: 200 OK
CLI-->>User: Added + SSL issued
Note over CLI,CoreAPI: scripts domains remove domain
User->>CLI: bunny scripts domains remove shop.example.com
CLI->>ComputeAPI: "GET /compute/script/{id}"
ComputeAPI-->>CLI: EdgeScriptModel
CLI->>CoreAPI: "GET /pullzone/{id} (verify ownership)"
CoreAPI-->>CLI: Hostname[]
CLI-->>User: Confirm remove?
User->>CLI: yes
CLI->>CoreAPI: "DELETE /pullzone/{id}/removeHostname"
CoreAPI-->>CLI: 200 OK
CLI-->>User: Removed shop.example.com
Reviews (3): Last reviewed commit: "switch to domains" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptile-apps re-review |
2 participants
Add a
scripts hostnamescommand group (add/ssl/list/remove) for managing a script's custom domains on its linked pull zone, via the core pull-zone API (addHostname/loadFreeCertificate/setForceSSL). SSL is opt-in on add; issuing a certificate forces HTTPS by default (--no-force-ssl to opt out).scripts deployandscripts shownow surface custom hostnames, deriving each URL's http/https scheme from its certificate / force-SSL state.