Conversation
avoid interference with --json
…r context handling
The slog term handler will truncate long lines.
Mostly EAFP
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors Azure BYOC/CD and blob storage access and wiring (ETag, job startup, upload URLs), changes Pulumi state parsing callback, updates stack label formatting and account derivation, adjusts several CLI empty-result messages to warnings, adds timing instrumentation, and migrates one debug logger to slog. ChangesAzure BYOC / CD and Blob Storage
Stack Selector, CLI messages, and Miscellaneous
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Byoc as ByocAzure
participant Driver as AzureDriver
participant Blob as AzureBlob
participant ACA as ACAJob
CLI->>Byoc: Deploy / CdCommand (cdCommand, env)
Byoc->>Byoc: SetUpCD(ctx) / setUpJob(ctx)
Byoc->>Driver: IterateBlobs(ctx, prefix) / DownloadBlob(ctx, key)
Driver->>Blob: List / Get / SAS sign (uses cached storageKey)
Blob-->>Driver: Blob metadata / bytes
Driver-->>Byoc: blob bytes / state.Info
Byoc->>ACA: StartJobExecution (env with ETAG, DEFANG_MODE, upload URLs)
ACA->>Blob: Log workspace lookup / key fetch
ACA-->>Byoc: job execution status / logs
Byoc-->>CLI: Job ID / ETag / result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.12.1)level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2." Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pkg/cli/client/byoc/aws/list.go (1)
77-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose each S3 object body after reading it.
GetObjectreturns a streaming body that must be closed. In this loop, leaking bodies will eventually pin connections and can makeListPulumiStacksstall or fail on buckets with many state files.Suggested fix
state, err := state.ParsePulumiStateFile(ctx, s3Obj{obj}, func(ctx context.Context, path string) ([]byte, error) { getObjectOutput, err := s3client.GetObject(ctx, &s3.GetObjectInput{ Bucket: &bucketName, Key: &path, }) if err != nil { return nil, err } - return io.ReadAll(getObjectOutput.Body) + defer getObjectOutput.Body.Close() + return io.ReadAll(getObjectOutput.Body) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/cli/client/byoc/aws/list.go` around lines 77 - 85, The loop calling s3client.GetObject inside the state.ParsePulumiStateFile call is leaking streaming bodies; after successfully calling s3client.GetObject (in the anonymous func passed to state.ParsePulumiStateFile) ensure you close getObjectOutput.Body (e.g., call getObjectOutput.Body.Close() via defer immediately after checking err == nil) so the HTTP connection is released; make sure the close runs regardless of subsequent io.ReadAll errors to avoid pinned connections when processing many s3Obj entries.src/pkg/clouds/azure/cd/blob.go (1)
26-44:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSynchronize the cached storage-key initialization.
CdListspawns goroutines that callDownloadBlobconcurrently (byoc.go:127), which invokesnewSharedKeyCredentialwithout synchronization. The unsynchronized reads and writes tod.storageKeyat lines 27–28 and 30–42 create a data race. Concurrent goroutines can also trigger duplicateListKeysAPI calls (line 35).Guard the lazy initialization with
sync.Onceor a mutex onDriver.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/clouds/azure/cd/blob.go` around lines 26 - 44, The Driver's lazy init of d.storageKey in newSharedKeyCredential is racy (used concurrently by CdList -> DownloadBlob). Protect the initialization by adding synchronization on Driver (e.g., a sync.Mutex or sync.Once field) so that reads/writes to storageKey are synchronized and only one goroutine calls NewStorageAccountsClient/ListKeys; update newSharedKeyCredential to check storageKey under the lock (or via Once) and perform the API call while holding the lock (or via the Once.Do) to avoid duplicate ListKeys calls and data races.
🧹 Nitpick comments (5)
src/pkg/stacks/selector_test.go (1)
556-663: ⚡ Quick winAdd a
DeployedAttable case to lock down timestamp label formatting.
stackLabelPartsnow includes a non-zero deployed-at branch; a dedicated table entry here would prevent silent regressions in label rendering.As per coding guidelines:
**/*_test.go: "Use table-driven tests for comprehensive test coverage in unit tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/stacks/selector_test.go` around lines 556 - 663, Add a table-driven test case to TestMakeStackSelectorLabels that covers the non-zero deployed-at branch by passing a ListItem with a populated DeployedAt timestamp and asserting makeStackSelectorLabels emits the expected timestamp fragment (matching the formatting produced by stackLabelParts); update the tests slice with a case name like "include deployed at timestamp" that supplies a deterministic DeployedAt value and the corresponding wantLabels entry so future changes to stackLabelParts's timestamp formatting will fail the test.src/pkg/stacks/manager_test.go (1)
650-832: ⚡ Quick winAdd a table case for account precedence/fallback in remote stacks.
Given the new
ListRemoteaccount logic, please add a case that asserts: (1) stack-file account wins when present, and (2)providerAccountIdis used when stack-file account is empty.As per coding guidelines:
**/*_test.go: "Use table-driven tests for comprehensive test coverage in unit tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/stacks/manager_test.go` around lines 650 - 832, Add a new table-driven case to TestGetStack that covers remote account precedence: create a case (e.g. name "remote stack account precedence") with options Default.Parameters{Name: "remotestack-account"}, set remoteStack to a Parameters instance that in one variant contains a stack-file account variable (e.g. remoteStack.Variables["ACCOUNT"] = "stack-file-account") and assert expectedEnv contains that account; add a second variant where the stack-file account is empty but the provider account id is present (simulate by setting remoteStack.Variables["PROVIDER_ACCOUNT_ID"] = "provider-account") and assert expectedEnv falls back to "provider-account". Place the case in the existing TestGetStack table alongside the other entries and set expectedStack/expectedEnv accordingly so the test validates that ListRemote account logic prefers the stack-file account and otherwise uses providerAccountId.src/pkg/cli/client/byoc/azure/byoc_test.go (1)
283-288: ⚡ Quick winDon't leave the new CDImage guard test skipped.
t.Skip("not sure")means this path is no longer covered, so a regression insetUpJob's precondition will slip through silently. Either assert the expected error now or drop the test until the behavior is defined.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/cli/client/byoc/azure/byoc_test.go` around lines 283 - 288, Remove the t.Skip in TestSetUpJobMissingCDImage and make the test assert the expected failure instead of being skipped: call newTestProvider(t, cloudazure.LocationEastUS, "sub") and invoke b.setUpJob(t.Context()), then require err != nil and optionally assert the error message mentions the missing CDImage (or matches the configured precondition error) so the precondition in setUpJob is exercised; if the intended behavior is undefined, remove the test entirely instead of skipping it.src/pkg/clouds/azure/cd/driver_test.go (1)
289-300: ⚡ Quick winKeep these assertions on the exported blob APIs.
Switching these tests to
iterateBlobsInContainer/downloadBlobFromContainerbypasses the new wrapper logic inblob.go, so regressions inIterateBlobsandDownloadBlobwould no longer be caught here. Prefer exercising the public entrypoints and reserve helper tests for helper-specific behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/clouds/azure/cd/driver_test.go` around lines 289 - 300, The tests are calling unexported helpers iterateBlobsInContainer and downloadBlobFromContainer which bypass the public wrapper in blob.go; change the calls to use the exported methods IterateBlobs and DownloadBlob on the driver (e.g. replace d.iterateBlobsInContainer(...) with d.IterateBlobs(...) and d.downloadBlobFromContainer(...) with d.DownloadBlob(...)), keep the same contexts/inputs and assertions expecting errors, and ensure the test exercises the public entrypoints so regressions in IterateBlobs and DownloadBlob are caught.src/pkg/cli/client/byoc/azure/byoc.go (1)
111-130: ⚡ Quick winUse structured
slogfields for the new blob-scan debug path.These new debug messages are string-formatted, so the blob name and error are harder to filter and they diverge from the repo logging rule. Switching this path to
slog.Debug/DebugContextwith named fields would keep it machine-readable.As per coding guidelines,
**/*.go: Use log/slog for structured debug logging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/cli/client/byoc/azure/byoc.go` around lines 111 - 130, Replace the unstructured term.Debug/term.Debugf calls in the blob-scan goroutine with structured slog logging (e.g., slog.Debug or slog.DebugContext) and emit named fields for the blob name and error so logs remain machine-readable; specifically update the messages around the iterator error and the ParsePulumiStateFile skip (the calls around state.ParsePulumiStateFile, the loop over blobs, and the use of item.Name()) to call slog.Debug/DebugContext with attributes like "blob": item.Name() and "err": err (or "reason": err) instead of string formatting, keeping the surrounding control flow (ctx/cancel, wg, stackCh, and b.driver.DownloadBlob) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pkg/cli/client/byoc/azure/byoc.go`:
- Around line 422-425: CdCommand/cdCommand may leave cmd.mode at the enum zero
value causing env["DEFANG_MODE"] to be unconditionally set to
"deployment_mode_unspecified"; change the logic so DEFANG_MODE is only added to
env when cmd.mode is explicitly set (i.e., not the enum zero/default). In other
words, guard the assignment of env["DEFANG_MODE"] (currently using
cmd.mode.String()) with a check like "if cmd.mode != 0" or the concrete enum's
UNSPECIFIED value so that existing default behavior is preserved when mode is
unset.
- Around line 117-147: The loop launches an unbounded goroutine per blob causing
unbounded concurrent b.driver.DownloadBlob calls; limit concurrency by
introducing a semaphore/bounded worker pool (e.g., a buffered channel sized to a
constant like maxParallel) and acquire it before starting the goroutine or
before calling DownloadBlob, then release it in a defer so only maxParallel
downloads run at once; keep existing wg, ctx cancellation,
state.ParsePulumiStateFile and sending to stackCh the same, but ensure the
semaphore is respected around the download/parse work to prevent Azure
throttling and memory spikes.
In `@src/pkg/cli/getServices.go`:
- Around line 107-112: The current check treats any colon in endpoint as a
signal to skip (causing endpoints like "example.com:443" to be marked skipped),
so replace the colon check with a scheme delimiter check: change the condition
that currently uses strings.Contains(endpoint, ":") to instead check for "://",
and only prepend "https://" when endpoint != "" && !strings.Contains(endpoint,
"://"); keep the else branch that sets *results[serviceInfo.Service.Name] =
"skipped" for truly invalid or non-HTTP schemes. This preserves allowing
schemeless host:port values while still defaulting bare hosts to HTTPS.
- Around line 112-124: Multiple goroutines and the main thread can concurrently
write to the same results map entry (results[serviceInfo.Service.Name]) — e.g.,
main thread writes "skipped" while spawned goroutines running RunHealthcheck
also assign to *results[serviceInfo.Service.Name], causing a race; protect all
reads/writes to the results map entry using a mutex (e.g., a sync.Mutex or
sync.RWMutex) or a per-service lock: acquire the lock before setting "skipped"
and also acquire it inside the goroutine before assigning result, then release
after the write so updates to results[serviceInfo.Service.Name] are serialized
while keeping wg, RunHealthcheck, and existing control flow unchanged.
In `@src/pkg/clouds/azure/aca/job.go`:
- Around line 202-211: The code dereferences
resp.Properties.AppLogsConfiguration without a nil check; update the Get
response handling in the SetUpManagedEnvironment flow (the envClient.Get block)
to first verify resp.Properties.AppLogsConfiguration != nil before assigning lc
:= *resp.Properties.AppLogsConfiguration and before accessing its fields
(Destination, LogAnalyticsConfiguration.CustomerID); if AppLogsConfiguration is
nil, skip this configured-check branch and continue creating/configuring the
environment, ensuring j.EnvironmentID is only set when resp.ID is valid and the
log-analytics matching condition succeeds.
---
Outside diff comments:
In `@src/pkg/cli/client/byoc/aws/list.go`:
- Around line 77-85: The loop calling s3client.GetObject inside the
state.ParsePulumiStateFile call is leaking streaming bodies; after successfully
calling s3client.GetObject (in the anonymous func passed to
state.ParsePulumiStateFile) ensure you close getObjectOutput.Body (e.g., call
getObjectOutput.Body.Close() via defer immediately after checking err == nil) so
the HTTP connection is released; make sure the close runs regardless of
subsequent io.ReadAll errors to avoid pinned connections when processing many
s3Obj entries.
In `@src/pkg/clouds/azure/cd/blob.go`:
- Around line 26-44: The Driver's lazy init of d.storageKey in
newSharedKeyCredential is racy (used concurrently by CdList -> DownloadBlob).
Protect the initialization by adding synchronization on Driver (e.g., a
sync.Mutex or sync.Once field) so that reads/writes to storageKey are
synchronized and only one goroutine calls NewStorageAccountsClient/ListKeys;
update newSharedKeyCredential to check storageKey under the lock (or via Once)
and perform the API call while holding the lock (or via the Once.Do) to avoid
duplicate ListKeys calls and data races.
---
Nitpick comments:
In `@src/pkg/cli/client/byoc/azure/byoc_test.go`:
- Around line 283-288: Remove the t.Skip in TestSetUpJobMissingCDImage and make
the test assert the expected failure instead of being skipped: call
newTestProvider(t, cloudazure.LocationEastUS, "sub") and invoke
b.setUpJob(t.Context()), then require err != nil and optionally assert the error
message mentions the missing CDImage (or matches the configured precondition
error) so the precondition in setUpJob is exercised; if the intended behavior is
undefined, remove the test entirely instead of skipping it.
In `@src/pkg/cli/client/byoc/azure/byoc.go`:
- Around line 111-130: Replace the unstructured term.Debug/term.Debugf calls in
the blob-scan goroutine with structured slog logging (e.g., slog.Debug or
slog.DebugContext) and emit named fields for the blob name and error so logs
remain machine-readable; specifically update the messages around the iterator
error and the ParsePulumiStateFile skip (the calls around
state.ParsePulumiStateFile, the loop over blobs, and the use of item.Name()) to
call slog.Debug/DebugContext with attributes like "blob": item.Name() and "err":
err (or "reason": err) instead of string formatting, keeping the surrounding
control flow (ctx/cancel, wg, stackCh, and b.driver.DownloadBlob) unchanged.
In `@src/pkg/clouds/azure/cd/driver_test.go`:
- Around line 289-300: The tests are calling unexported helpers
iterateBlobsInContainer and downloadBlobFromContainer which bypass the public
wrapper in blob.go; change the calls to use the exported methods IterateBlobs
and DownloadBlob on the driver (e.g. replace d.iterateBlobsInContainer(...) with
d.IterateBlobs(...) and d.downloadBlobFromContainer(...) with
d.DownloadBlob(...)), keep the same contexts/inputs and assertions expecting
errors, and ensure the test exercises the public entrypoints so regressions in
IterateBlobs and DownloadBlob are caught.
In `@src/pkg/stacks/manager_test.go`:
- Around line 650-832: Add a new table-driven case to TestGetStack that covers
remote account precedence: create a case (e.g. name "remote stack account
precedence") with options Default.Parameters{Name: "remotestack-account"}, set
remoteStack to a Parameters instance that in one variant contains a stack-file
account variable (e.g. remoteStack.Variables["ACCOUNT"] = "stack-file-account")
and assert expectedEnv contains that account; add a second variant where the
stack-file account is empty but the provider account id is present (simulate by
setting remoteStack.Variables["PROVIDER_ACCOUNT_ID"] = "provider-account") and
assert expectedEnv falls back to "provider-account". Place the case in the
existing TestGetStack table alongside the other entries and set
expectedStack/expectedEnv accordingly so the test validates that ListRemote
account logic prefers the stack-file account and otherwise uses
providerAccountId.
In `@src/pkg/stacks/selector_test.go`:
- Around line 556-663: Add a table-driven test case to
TestMakeStackSelectorLabels that covers the non-zero deployed-at branch by
passing a ListItem with a populated DeployedAt timestamp and asserting
makeStackSelectorLabels emits the expected timestamp fragment (matching the
formatting produced by stackLabelParts); update the tests slice with a case name
like "include deployed at timestamp" that supplies a deterministic DeployedAt
value and the corresponding wantLabels entry so future changes to
stackLabelParts's timestamp formatting will fail the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5de98887-564e-4b14-983a-3117384fd9e4
📒 Files selected for processing (26)
src/cmd/cli/command/stack.gosrc/cmd/cli/command/stack_test.gosrc/cmd/cli/command/workspace.gosrc/pkg/agent/plugins/compat_oai/generate.gosrc/pkg/cli/cd.gosrc/pkg/cli/client/byoc/aws/list.gosrc/pkg/cli/client/byoc/azure/byoc.gosrc/pkg/cli/client/byoc/azure/byoc_test.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/byoc/state/parse.gosrc/pkg/cli/client/byoc/state/parse_test.gosrc/pkg/cli/getServices.gosrc/pkg/clouds/azure/aca/job.gosrc/pkg/clouds/azure/aca/job_test.gosrc/pkg/clouds/azure/cd/blob.gosrc/pkg/clouds/azure/cd/driver.gosrc/pkg/clouds/azure/cd/driver_test.gosrc/pkg/clouds/azure/cd/setup.gosrc/pkg/clouds/azure/cd/upload.gosrc/pkg/clouds/azure/common.gosrc/pkg/stacks/manager.gosrc/pkg/stacks/manager_test.gosrc/pkg/stacks/selector.gosrc/pkg/stacks/selector_test.gosrc/pkg/stacks/stacks.gosrc/pkg/term/timing.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pkg/cli/client/byoc/state/parse.go (1)
51-51: ⚡ Quick winUse structured
slogfor the debug log statement.Line 51 uses
term.Debugf, which is unstructured logging and diverges from the coding guideline requiringlog/slogfor structured debug logging in Go files. Switch toslog.DebugContextwith keyed attributes ("object"and"size_bytes").Proposed change
import ( "context" "encoding/json" "fmt" + "log/slog" "path" "strconv" "strings" @@ - term.Debugf("loading Pulumi state from %q (size %d bytes)", obj.Name(), obj.Size()) + slog.DebugContext(ctx, "loading Pulumi state", "object", obj.Name(), "size_bytes", obj.Size())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/cli/client/byoc/state/parse.go` at line 51, Replace the unstructured term.Debugf call with a structured slog call: use slog.DebugContext(ctx, ...) instead of term.Debugf and pass keyed attributes for the object name and size (e.g. slog.String("object", obj.Name()) and slog.Int64("size_bytes", obj.Size())). Update imports to include "log/slog" and ensure a context variable (ctx) is in scope where the original term.Debugf is called; locate the call to term.Debugf in parse.go and replace it with slog.DebugContext using obj.Name() and obj.Size() as the attribute values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pkg/clouds/azure/aca/job.go`:
- Around line 141-143: In setUpLogWorkspace, the wsClient.Get success branch
dereferences resp.Properties.CustomerID without nil checks; update the Get
branch (response from wsClient.Get) to first verify resp.Properties != nil and
resp.Properties.CustomerID != nil before dereferencing, and only then assign
customerID and log; mirror the defensive checks used in the CreateOrUpdate
branch so you avoid a potential nil pointer dereference when calling
resp.Properties.CustomerID.
---
Nitpick comments:
In `@src/pkg/cli/client/byoc/state/parse.go`:
- Line 51: Replace the unstructured term.Debugf call with a structured slog
call: use slog.DebugContext(ctx, ...) instead of term.Debugf and pass keyed
attributes for the object name and size (e.g. slog.String("object", obj.Name())
and slog.Int64("size_bytes", obj.Size())). Update imports to include "log/slog"
and ensure a context variable (ctx) is in scope where the original term.Debugf
is called; locate the call to term.Debugf in parse.go and replace it with
slog.DebugContext using obj.Name() and obj.Size() as the attribute values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 547e6e10-2ef7-4626-a469-9ab34de2e4ba
📒 Files selected for processing (7)
src/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/list.gosrc/pkg/cli/client/byoc/azure/byoc.gosrc/pkg/cli/client/byoc/state/parse.gosrc/pkg/clouds/azure/aca/job.gosrc/pkg/clouds/azure/cd/blob.gosrc/pkg/clouds/azure/cd/driver.go
✅ Files skipped from review due to trivial changes (3)
- src/pkg/cli/client/byoc/aws/byoc.go
- src/pkg/clouds/azure/cd/blob.go
- src/pkg/cli/client/byoc/aws/list.go
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pkg/clouds/azure/cd/driver.go
- src/pkg/cli/client/byoc/azure/byoc.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/protos/io/defang/v1/fabric.proto (1)
586-592:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd deprecated alias to maintain backward compatibility for external clients.
The enum constant was renamed from
CD_TYPE_AZURE_ACI_JOBIDtoCD_TYPE_AZURE_ACA_JOBIDin commit 642dda8. While the numeric value (4) preserves wire compatibility, removing the old constant name breaks external clients that depend on the original constant. Existing external code referencingCdType_CD_TYPE_AZURE_ACI_JOBIDwill fail to compile.Add a deprecated alias in a follow-up change to allow graceful migration:
Suggested backward-compatible fix
enum CdType { + option allow_alias = true; CD_TYPE_UNSPECIFIED = 0; CD_TYPE_AWS_CODEBUILD_BUILDID = 1; CD_TYPE_GCP_CLOUDBUILD_BUILDID = 2; CD_TYPE_DO_APPPLATFORM_DEPLOYMENTID = 3; + CD_TYPE_AZURE_ACI_JOBID = 4 [deprecated = true]; CD_TYPE_AZURE_ACA_JOBID = 4; }Then regenerate protos:
cd src && make protos🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/protos/io/defang/v1/fabric.proto` around lines 586 - 592, Add a deprecated alias for the renamed enum value so external clients referencing the old name still compile: in the enum CdType add the old constant name CD_TYPE_AZURE_ACI_JOBID with the same numeric value as CD_TYPE_AZURE_ACA_JOBID (4) and mark it deprecated (e.g., CD_TYPE_AZURE_ACI_JOBID = 4 [deprecated = true]); then regenerate protos (cd src && make protos).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/protos/io/defang/v1/fabric.proto`:
- Line 591: fabric.proto was modified (added CD_TYPE_AZURE_ACA_JOBID = 4), but
the checked-in generated protobuf artifacts were not updated; regenerate the
derived files by running the project codegen (cd src && make protos) to produce
updated outputs for all src/protos/**/*.proto, verify that generated files
reflect the new CD_TYPE_AZURE_ACA_JOBID constant, add those regenerated
artifacts to this PR, and run the build/tests to ensure consumers compile
against the new schema.
---
Outside diff comments:
In `@src/protos/io/defang/v1/fabric.proto`:
- Around line 586-592: Add a deprecated alias for the renamed enum value so
external clients referencing the old name still compile: in the enum CdType add
the old constant name CD_TYPE_AZURE_ACI_JOBID with the same numeric value as
CD_TYPE_AZURE_ACA_JOBID (4) and mark it deprecated (e.g.,
CD_TYPE_AZURE_ACI_JOBID = 4 [deprecated = true]); then regenerate protos (cd src
&& make protos).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 238a3f1b-0837-4b7a-91ef-7e28403ae66f
⛔ Files ignored due to path filters (1)
src/protos/io/defang/v1/fabric.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
src/go.modsrc/pkg/cli/client/byoc/azure/byoc.gosrc/protos/io/defang/v1/fabric.proto
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
2 participants
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.
Description
term.Timing()helper for profiling bootstrap performanceslogfor AI agent debugging (avoid super long debug messages)pulumi-defangwrites""to worker endpoint output)term.Warnfor "no stacks", "no projects", "no configs", etc.Linked Issues
Checklist
Summary by CodeRabbit
Bug Fixes
Improvements
Refactor
Tests