fix(cli): respect deployment intent and make template seeding retry-safe - #1823
Merged
Merged
Conversation
A settings-only push (--no-code, or a declined deployment confirmation) of a pending GitHub-backed function created the repository anyway and cleared the pending intent, leaving an unwanted empty repository. Repository materialization now runs only once a deployment is actually requested, and a settings write for a still-pending function omits the VCS connection keys so an installation is never sent without its repository. The one-shot template coordinates were cleared only after a successful template deployment, so a deployment that succeeded remotely while the local write failed re-submitted the template on retry and seeded the starter twice. They are now cleared and persisted before the request, and restored when the request fails.
Contributor
Greptile SummaryThe PR delays pending GitHub repository materialization until deployment is approved and consumes template-seeding state before issuing the deployment request. It also serializes batches containing template deployments, but currently loses the seed coordinates after a definite API rejection.
Confidence Score: 4/5The PR is not yet safe to merge because a definite template-deployment rejection permanently discards the coordinates needed for retry. A non-timeout 4xx response proves that the template request was rejected, yet the new unconditional error path keeps the pre-consumed seed state persisted; the next push therefore switches to a normal branch deployment and never seeds the starter. Files Needing Attention: templates/cli/internal/cmd/pushdeploy.go Important Files Changed
Prompt To Fix All With AI### Issue 1
templates/cli/internal/cmd/pushdeploy.go:1479-1487
**Definite rejections consume template state**
When the template endpoint definitively rejects a deployment with a non-timeout 4xx response, this branch leaves the pre-request deletion persisted. The next push therefore selects `/deployments/vcs` instead of retrying the template deployment, so the starter template is never seeded.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (8): Last reviewed commit: "refactor(cli): simplify deployment inten..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Repository materialization moves into the per-entry push path, after the settings write, endpoint rule, and variable replacement have all succeeded and a deployment is definitely being submitted; a failure in any earlier step can no longer leave an unwanted empty GitHub repository. The function is connected to the repository with a follow-up settings write once it exists. Template coordinates are restored only on a definite server rejection. A timeout, read, or decode failure does not say whether the deployment was created, and a consumed seed cannot merge the starter twice -- a restored one can.
…re seeds only on 4xx The pending flag now survives repository materialization and is cleared only after the deployment that needed the repository was submitted, so a failed VCS connection or deployment leaves a state the next push fully retries -- creation falls back to finding the repository by name -- instead of an orphaned repository nothing owns. Template coordinates are restored only on a definite 4xx rejection. A 5xx can arrive after the deployment was accepted, so it keeps the seed consumed exactly like a transport failure.
…emplate seed An intermediary can emit 408 after the deployment was accepted upstream, so it keeps the seed consumed exactly like a transport failure or a 5xx.
Clearing the pending flag after a successful deployment meant a failed confirmation write left the configuration pending on disk, so the next push replayed a submission that had already succeeded. Once the VCS connection PUT lands the pending flow has nothing left to redo, so the flag is cleared then -- a failed confirmation now stops the push before anything is submitted.
…on workers Parallel pending-function pushes share one config.Local; an unsynchronized upsert-and-write could overwrite another worker's repository or template state. All function entry persistence now goes through a mutex-guarded persistEntry.
Serialization reads every entry, so a Set or Delete outside the lock could race a Write another worker holds the lock for. persistEntry and upsertEntry now take the mutation as a closure and run it inside the critical section.
Comment on lines
+1479
to
1487
| if err := c.api.Call("POST", path, body, deployment); err != nil { | ||
| if template { | ||
| return nil, fmt.Errorf( | ||
| "%w; template state was consumed before the request -- check the function's deployments before retrying", | ||
| err) | ||
| } | ||
|
|
||
| return nil, err | ||
| } |
Contributor
There was a problem hiding this comment.
When the template endpoint definitively rejects a deployment with a non-timeout 4xx response, this branch leaves the pre-request deletion persisted. The next push therefore selects /deployments/vcs instead of retrying the template deployment, so the starter template is never seeded.
Knowledge Base Used: CLI distribution template
Prompt To Fix With AI
This is a comment left during a code review.
Path: templates/cli/internal/cmd/pushdeploy.go
Line: 1479-1487
Comment:
**Definite rejections consume template state**
When the template endpoint definitively rejects a deployment with a non-timeout 4xx response, this branch leaves the pre-request deletion persisted. The next push therefore selects `/deployments/vcs` instead of retrying the template deployment, so the starter template is never seeded.
**Knowledge Base Used:** [CLI distribution template](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/sdk-generator/-/docs/cli-distribution-template.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the two P1 findings Greptile raised on appwrite/sdk-for-cli#360.
Repository created before deployment intent. Repository materialization now runs only after code deployment is confirmed. A settings-only push (
--no-code, or a declined deployment confirmation) keeps the repository pending and omits VCS connection fields from the settings request.Template seed state is not atomic. One-shot template coordinates are cleared and persisted before the deployment request. If persistence fails, no request is made. Once a request is attempted, the coordinates remain consumed because the remote outcome may be ambiguous; this prevents a retry from seeding the starter twice.
Template-seeded batches run sequentially while ordinary function pushes remain parallel, avoiding concurrent writes to the shared local configuration without adding general locking infrastructure.
Verified by regenerating the CLI and running
gofmt,go build ./...,go vet ./..., andgo test ./...against the generated output.composer lint-twigandcomposer refactor:checkalso pass.