Add GetUploadURL API and add 2 upload urls to Deploy call for Provider#1751
Add GetUploadURL API and add 2 upload urls to Deploy call for Provider#1751
Conversation
📝 WalkthroughWalkthroughAdded two new string fields (stack, filename) to UploadURLRequest in the protobuf and replaced several instances of direct string concatenation with strings.Builder in three Go files. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-01-07T00:34:13.131ZApplied to files:
🧬 Code graph analysis (1)src/pkg/cli/client/byoc/parse.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/protos/io/defang/v1/fabric.proto (2)
154-156: Consider adding documentation for the new RPC.The new
GetUploadURLRPC lacks documentation explaining its purpose, parameters, and expected usage. Consider adding comments to clarify:
- What type of upload this URL is for (states, events, or generic)
- How it relates to the new
states_upload_urlandevents_upload_urlfields inDeployRequest- Whether the
nameparameter in the request specifies the upload type📝 Example documentation
+ // GetUploadURL returns a pre-signed URL for uploading deployment artifacts + // (states or events). The 'name' parameter specifies the artifact type. rpc GetUploadURL(GetUploadURLRequest) returns (GetUploadURLResponse) { option idempotency_level = NO_SIDE_EFFECTS; };
649-655: Consider documenting the request/response messages.The purpose of the
GetUploadURLRequestfields (especially thenamefield) and the intended use of the returned URL are unclear without documentation.📝 Example documentation
+// GetUploadURLRequest requests a pre-signed upload URL for deployment artifacts. message GetUploadURLRequest { - string project = 1; - string stack = 2; - string name = 3; + string project = 1; // Project identifier + string stack = 2; // Stack identifier + string name = 3; // Artifact type (e.g., "states", "events") } +// GetUploadURLResponse contains the pre-signed upload URL. message GetUploadURLResponse { string url = 1; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/protos/io/defang/v1/fabric.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (4)
src/pkg/cli/client/client.gosrc/pkg/cli/client/grpc.gosrc/protos/io/defang/v1/defangv1connect/fabric.connect.gosrc/protos/io/defang/v1/fabric.proto
🧰 Additional context used
🧬 Code graph analysis (3)
src/pkg/cli/client/grpc.go (1)
src/protos/io/defang/v1/fabric.pb.go (6)
GetUploadURLRequest(4540-4547)GetUploadURLRequest(4560-4560)GetUploadURLRequest(4575-4577)GetUploadURLResponse(4600-4605)GetUploadURLResponse(4618-4618)GetUploadURLResponse(4633-4635)
src/pkg/cli/client/client.go (1)
src/protos/io/defang/v1/fabric.pb.go (6)
GetUploadURLRequest(4540-4547)GetUploadURLRequest(4560-4560)GetUploadURLRequest(4575-4577)GetUploadURLResponse(4600-4605)GetUploadURLResponse(4618-4618)GetUploadURLResponse(4633-4635)
src/protos/io/defang/v1/defangv1connect/fabric.connect.go (1)
src/protos/io/defang/v1/fabric.pb.go (6)
GetUploadURLRequest(4540-4547)GetUploadURLRequest(4560-4560)GetUploadURLRequest(4575-4577)GetUploadURLResponse(4600-4605)GetUploadURLResponse(4618-4618)GetUploadURLResponse(4633-4635)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: nix-shell-test
🔇 Additional comments (4)
src/protos/io/defang/v1/fabric.proto (1)
265-266: LGTM! Safe field numbering for backward compatibility.The new fields use sequential field numbers (9, 10) which maintains backward compatibility. The naming is clear and follows proto naming conventions.
src/pkg/cli/client/grpc.go (1)
198-200: LGTM! Implementation follows established patterns.The
GetUploadURLmethod correctly follows the same delegation pattern used by other methods likeGenerateCompose, usingconnect.NewRequestand thegetMsghelper function.src/pkg/cli/client/client.go (1)
26-26: LGTM! Interface method properly defined.The method signature is consistent with other interface methods and properly positioned alphabetically.
src/protos/io/defang/v1/defangv1connect/fabric.connect.go (1)
160-162: LGTM! Generated code correctly implements the new RPC.The auto-generated Connect code properly wires the
GetUploadURLRPC with:
- Correct procedure constant definition
- Client and handler interface methods
- Proper idempotency level (
NO_SIDE_EFFECTS) matching the proto definition- HTTP route handling
- Unimplemented handler stub
All changes follow the established patterns for other RPCs.
Also applies to: 220-220, 477-482, 531-531, 761-764, 821-821, 1074-1079, 1168-1169, 1351-1353
jordanstephens
left a comment
jordanstephens
left a comment
There was a problem hiding this comment.
This looks reasonable to me
edwardrf
force-pushed
the
edw/add-upload-url-api
branch
from
cc602fc to
82c493c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pkg/cli/client/byoc/gcp/byoc.go (1)
844-854: Efficient string building with strings.Builder.The refactoring correctly uses strings.Builder to assemble log entries, improving performance over string concatenation.
⚡ Optional micro-optimization
To maximize efficiency, consider writing each component separately instead of concatenating before writing:
- result.WriteString(logTimestamp + " " + msg + "\n") + result.WriteString(logTimestamp) + result.WriteString(" ") + result.WriteString(msg) + result.WriteString("\n")This avoids intermediate string allocations, though the current implementation is already a significant improvement over the previous approach.
src/pkg/cli/client/byoc/parse.go (1)
27-41: Improved string building for pending operations.The refactoring correctly uses strings.Builder for the pending portion, improving performance when multiple pending operations exist.
⚡ Optional micro-optimization
To maximize efficiency, consider writing each component separately:
- pending.WriteString(" " + strconv.Quote(p)) + pending.WriteString(" ") + pending.WriteString(strconv.Quote(p))This avoids intermediate string allocations, though the current implementation is already a good improvement.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/protos/io/defang/v1/fabric.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (4)
src/pkg/agent/plugins/compat_oai/generate.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/byoc/parse.gosrc/protos/io/defang/v1/fabric.proto
🚧 Files skipped from review as they are similar to previous changes (1)
- src/protos/io/defang/v1/fabric.proto
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/pkg/cli/client/byoc/parse.gosrc/pkg/agent/plugins/compat_oai/generate.gosrc/pkg/cli/client/byoc/gcp/byoc.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
🔇 Additional comments (2)
src/pkg/agent/plugins/compat_oai/generate.go (2)
22-22: LGTM! Import added for strings.Builder refactor.The
stringsimport is correctly added to support the strings.Builder usage in the refactored concatenateContent function.
247-253: Efficient string concatenation using strings.Builder.The refactoring from string concatenation to strings.Builder is correct and improves performance, especially when concatenating multiple parts in a loop.
4 participants
Description
To allow fabric to provide locations to upload state and events for later analysis in the UI.
This is step 1 where the gRPC is defined, the following implementation will be done in fabric, and once that is available, cli will implement using of the new API
Linked Issues
https://github.com/DefangLabs/defang-mvp/issues/2562
Checklist
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.