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 adds a
Confidence Score: 4/5Safe to merge with the interactive-path link fix applied; the remaining issue is a defensive guard for an edge case that is unlikely in production but leaves the manifest in a broken state if it fires. The interactive path of packages/cli/src/commands/pz/link.ts — interactive selection path at line 107 needs a null/undefined guard on Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
CLI[bunny pz] --> LIST[pz list]
CLI --> CREATE[pz create]
CLI --> LINK[pz link]
CLI --> UNLINK[pz unlink]
CLI --> SHOW[pz show]
CLI --> PURGE[pz purge]
CLI --> DELETE[pz delete]
LINK --> MANIFEST[(pullzone.json)]
CREATE -->|optionally| MANIFEST
UNLINK --> MANIFEST
DELETE -->|if linked zone| MANIFEST
MANIFEST -->|resolves id| SHOW
MANIFEST -->|resolves id| PURGE
MANIFEST -->|resolves id| DELETE
LIST -->|GET /pullzone| API[Bunny API]
CREATE -->|POST /pullzone| API
LINK -->|GET /pullzone or /pullzone-id| API
SHOW -->|GET /pullzone-id| API
PURGE -->|POST /pullzone-id/purgeCache| API
DELETE -->|DELETE /pullzone-id| API
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
CLI[bunny pz] --> LIST[pz list]
CLI --> CREATE[pz create]
CLI --> LINK[pz link]
CLI --> UNLINK[pz unlink]
CLI --> SHOW[pz show]
CLI --> PURGE[pz purge]
CLI --> DELETE[pz delete]
LINK --> MANIFEST[(pullzone.json)]
CREATE -->|optionally| MANIFEST
UNLINK --> MANIFEST
DELETE -->|if linked zone| MANIFEST
MANIFEST -->|resolves id| SHOW
MANIFEST -->|resolves id| PURGE
MANIFEST -->|resolves id| DELETE
LIST -->|GET /pullzone| API[Bunny API]
CREATE -->|POST /pullzone| API
LINK -->|GET /pullzone or /pullzone-id| API
SHOW -->|GET /pullzone-id| API
PURGE -->|POST /pullzone-id/purgeCache| API
DELETE -->|DELETE /pullzone-id| API
Reviews (11): Last reviewed commit: "Merge branch 'main' into pullzone-comman..." | 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)
jamie-at-bunny
left a comment
jamie-at-bunny
left a comment
There was a problem hiding this comment.
Awesome updates here @burstw0w, I'll get round to fully reviewing, and merging this Thursday/Friday. Thanks for your patience, awesome contribution 😻
|
@burstw0w hey, i took this for a spin locally. It checks out, and does what we need. Do you have any objections to me pushing a few commits here? One thing I'd like to implement is the prompt fallback if args/flags aren't provided, instead of an error response args are missing, it's nice to just ask for them from a DX point of view. |
|
@jamie-at-bunny Feel free to do, or request any changes you want. |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
| saveManifest<PullZoneManifest>(PULL_ZONE_MANIFEST, { | ||
| id: selected.Id, | ||
| }); |
There was a problem hiding this comment.
selected.Id saved without nullish fallback
In the interactive path, selected.Id (typed as number | null | undefined from PullZoneModel) is written directly to the manifest. If the API ever returns a zone without an Id, the manifest is saved as { id: undefined }. Every subsequent command that reads the manifest — pz show, pz purge, pz delete — resolves zoneId as undefined and throws "No pull zone specified", making the linked context appear broken for no clear reason.
The direct-ID path already has the right pattern: id: zone.Id ?? id. The interactive path should apply the same guard before writing the manifest. Compare with link.ts line 59.
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