Added basic PZ commands#86
Conversation
|
@codex review |
|
![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: 54b9cbc720
ℹ️ 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".
|
@bogdan-at-bunny Resolved all the issues codex found. Can you run it again? |
jamie-at-bunny
left a comment
jamie-at-bunny
left a comment
There was a problem hiding this comment.
Hey @burstw0w, appreciate the PR and enthusiasm here ❤️
It's a solid first pass. I appreciate the thought of keeping existing patterns in other commands here in this one.
That said, we should cut the scope back to "billy basic". I would drop clone, use id instead of name sugar+magic, and use the type system we already have to ensure end to end type safety as much as possible. This will result in less code added, and more predictable results. We're planning to add a project schema soon where a Project schema owns multiple PZs, at which point name based lookups get ambiguous and the manifest likely needs to carry project context too. Let's anchor on the stable IDs now, and provide the link/unlink methods for single PZ contexts.
I left a few raw comments as I went through the code. Apologies for the delay in reviewing, my focus is purely on scripts and apps at the moment (and storage next), amongst other things.
| type PullZoneManifest, | ||
| } from "./constants.ts"; | ||
|
|
||
| interface PullZone { |
There was a problem hiding this comment.
I'd prefer we didn't new interfaces and used what we could from the OpenAPI Spec, components["schemas"]["PullZoneModel"] should have what's needed here.
| Name?: string | null; | ||
| } | ||
|
|
||
| interface CreateArgs { |
There was a problem hiding this comment.
components["schemas"]["EdgeRuleV2Model"] should be used inline instead of declaring here, at worst, we can Pick from here, although I'd rather not limit scope and use the input type as is, which allows us to spread on more options.
| const createSpin = spinner("Creating pull zone..."); | ||
| createSpin.start(); | ||
|
|
||
| await client.POST("/pullzone", { |
There was a problem hiding this comment.
What happens here if there is an error from the API?
There was a problem hiding this comment.
Rewrote process of creating PZ completely, it now has a check for that as well.
| const spin = spinner("Purging cache..."); | ||
| spin.start(); | ||
|
|
||
| await client.POST("/pullzone/{id}/purgeCache", { |
There was a problem hiding this comment.
What happens with any error/response from the API here? Network or response error.
There was a problem hiding this comment.
Nothing, you are correct.
Added a check that will print an error if there is any.
|
Scaled everything down a bit, and implemented most of fixes requested by @jamie-at-bunny |
|
@greptile-apps review |
Greptile SummaryThis PR introduces the
Confidence Score: 4/5Safe to merge with the try/catch error-handling fix applied to show.ts and link.ts; all other issues are already tracked in prior review threads. In show.ts and the id path of link.ts, the openapi client's HTTP errors land in the { error } field and are never inspected — the try/catch only catches true network exceptions. A 401 or 5xx response leaves zone undefined and the user receives a misleading 'not found' message instead of the real failure reason. packages/cli/src/commands/pz/show.ts and packages/cli/src/commands/pz/link.ts (id path) need the try/catch replaced with { data, error } destructuring, matching the pattern already used in delete.ts. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
CLI["bunny pz"] --> LIST["pz list\nGET /pullzone"]
CLI --> CREATE["pz create name origin\nPOST /pullzone"]
CLI --> DELETE["pz delete [id]\nGET + DELETE /pullzone/{id}"]
CLI --> LINK["pz link [id]"]
CLI --> UNLINK["pz unlink"]
CLI --> PURGE["pz purge [id]\nPOST /pullzone/{id}/purgeCache"]
CLI --> SHOW["pz show [id]\nGET /pullzone/{id}"]
LINK -->|id supplied| LINK_ID["GET /pullzone/{id}\ntry/catch only"]
LINK -->|interactive| LINK_INT["GET /pullzone\nselect prompt"]
SHOW --> SHOW_GET["GET /pullzone/{id}\ntry/catch only"]
DELETE --> DEL_GET["GET /pullzone/{id}\n{ data, error } check"]
DEL_GET --> CONFIRM["confirm prompt"]
CONFIRM -->|ok| DEL_DELETE["DELETE /pullzone/{id}"]
DEL_DELETE --> MANIFEST_RM["removeManifest if linked"]
CREATE --> CREATE_POST["POST /pullzone\n{ data, error } check"]
CREATE_POST --> MANIFEST_OPT["offer saveManifest"]
LINK_INT --> MANIFEST_SAVE["saveManifest pullzone.json"]
LINK_ID --> MANIFEST_SAVE
UNLINK --> MANIFEST_DEL["removeManifest pullzone.json"]
Reviews (8): Last reviewed commit: "Added error catch" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const { data, error } = await client.POST("/pullzone", { | ||
| body: { Name: name, OriginUrl: url } as any, | ||
| }); | ||
|
|
||
| createSpin.stop(); | ||
|
|
||
| if (error) { | ||
| throw new UserError(`Failed to create pull zone: ${error}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
as any cast and inline type instead of generated schema types
The POST body is cast with as any and the response is typed with an inline { Id?: number; Name?: string | null } rather than using Pick<components["schemas"]["PullZone"], ...> from @bunny.net/openapi-client/generated/core.d.ts. CLAUDE.md explicitly requires: "Prefer generated schema types over inline primitives. Only fall back to string, any, or number when no generated type exists." Both casts bypass compile-time safety, so a field rename in the spec would silently break this command.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| import { defineNamespace } from "../../core/define-namespace.ts"; | ||
| import { pzCreateCommand } from "./create.ts"; | ||
| import { pzDeleteCommand } from "./delete.ts"; | ||
| import { pzLinkCommand } from "./link.ts"; | ||
| import { pzListCommand } from "./list.ts"; | ||
| import { pzPurgeCommand } from "./purge.ts"; | ||
| import { pzShowCommand } from "./show.ts"; | ||
| import { pzUnlinkCommand } from "./unlink.ts"; | ||
|
|
||
| // TODO: implement rules and hostnames subcommands | ||
| // const rulesNamespace = defineNamespace("rules", "Manage pull zone edge rules.", [...]); | ||
| // const hostnamesNamespace = defineNamespace("hostnames", "Manage pull zone hostnames.", [...]); | ||
|
|
||
| export const pzNamespace = defineNamespace( | ||
| "pz", | ||
| "Manage pull zones.", | ||
| [ | ||
| pzListCommand, | ||
| pzCreateCommand, | ||
| pzDeleteCommand, | ||
| pzLinkCommand, | ||
| pzPurgeCommand, | ||
| pzShowCommand, | ||
| pzUnlinkCommand, | ||
| ], |
There was a problem hiding this comment.
README.md and AGENTS.md not updated
CLAUDE.md explicitly requires: "When adding, changing, or removing commands or flags, update the corresponding sections in README.md — user-facing command docs and examples. AGENTS.md — architecture docs, command reference tree, and file listing." The new pz namespace and its seven sub-commands are absent from both documents.
Context Used: CLAUDE.md (source)
| const { data } = await client.GET("/pullzone"); | ||
|
|
||
| spin.stop(); | ||
|
|
||
| const zones = ((data ?? []) as PullZone[]).sort((a: PullZone, b: PullZone) => |
There was a problem hiding this comment.
client.GET("/pullzone") returns { data, error } but only data is destructured. When the request fails (invalid API key, network error, server 5xx), data is undefined, so zones becomes [] and the command prints "No pull zones found." instead of surfacing the real error. The same pattern appears in the interactive path of link.ts at the equivalent GET /pullzone call — both should destructure and check error.
There was a problem hiding this comment.
Shouldn't middleware throw every non ok response before destructuring? I mostly transferred my golang code to ts, but i've been trying to do stuff like you do it in db. I might have missed something
3 participants
Right now, you can do:
pz select - selects a pullzone, keeping it in context so all the other commands can be ran without specifying ID/name
pz deselect - removes context (this might not be needed, as you dont have that for your db commands)
pz create - creates a pullzone, accepts two params and those are [name] [origin url]
pz delete - deletes pullzone (if you don't use name, it will ask you if you want to delete the current zone that is in the context)
pz purge - purges pullzone (again, if you dont specify the name, uses context)
pz show - shows info about pullzone
pz list - lists your available pullzones
pz clone - clones your pullzone
Tested everything with bun run