Azure: improve logging#264
Draft
lionello wants to merge 4 commits into
Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR restructures the CD command entrypoint from global environment variables into a ChangesCD Command Entrypoint Refactoring
Azure Provider Resource Parenting Refactoring
Azure Container Apps Logging Architecture
Test Fixtures and Configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 unit tests (beta)
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: 7
🧹 Nitpick comments (2)
.claude/settings.json (1)
7-21: ⚡ Quick winTighten the new Azure CLI allowlist.
These additions permit broad subscription-wide enumeration (
account list,group list, workspace/tablelist, and role-assignment reads), which expands the assistant’s read scope beyond the specific integration resources under test. Prefer the narrowestshowor resource-scoped commands you can support here.🤖 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 @.claude/settings.json around lines 7 - 21, The allowlist currently includes broad Azure CLI "list" commands (e.g., "Bash(az account list:*)", "Bash(az group list:*)", "Bash(az monitor log-analytics workspace table list:*)", "Bash(az role assignment list:*)") which enable subscription-wide enumeration; replace those with the narrowest resource-scoped or "show" variants (or remove them) so the assistant only has read access to the specific integration resources under test—update the listed entries (for example, keep "Bash(az account show:*)", "Bash(az group show:*)", "Bash(az monitor log-analytics workspace show:*)", "Bash(az monitor log-analytics workspace table show:*)", and remove or tighten the corresponding "list" and "role assignment list" lines) to restrict scope.cd/run.sh (1)
4-5: ⚡ Quick winUse per-run temp files for upload artifacts.
Fixed
/tmp/events-azure.jsonand/tmp/states-azure.jsonpaths will collide across parallel or repeated runs, which makes the integration output easy to clobber or misread. Generate unique files per invocation instead.Suggested change
-export DEFANG_EVENTS_UPLOAD_URL=file:///tmp/events-azure.json -export STATES_EVENTS_UPLOAD_URL=file:///tmp/states-azure.json +events_file="$(mktemp /tmp/events-azure.XXXXXX.json)" +states_file="$(mktemp /tmp/states-azure.XXXXXX.json)" +export DEFANG_EVENTS_UPLOAD_URL="file://${events_file}" +export STATES_EVENTS_UPLOAD_URL="file://${states_file}"🤖 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 `@cd/run.sh` around lines 4 - 5, The environment variables DEFANG_EVENTS_UPLOAD_URL and STATES_EVENTS_UPLOAD_URL currently point to fixed files (/tmp/events-azure.json and /tmp/states-azure.json) which will collide across parallel or repeated runs; change the script to generate unique per-run temp file paths (e.g., via mktemp or by including $$ and a timestamp) and assign those generated file paths to DEFANG_EVENTS_UPLOAD_URL and STATES_EVENTS_UPLOAD_URL so each invocation writes to its own artifact file.
🤖 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 `@cd/cd_main.go`:
- Around line 36-46: The code currently validates projectName and stackName
before parsing the command, causing `cd help` or empty-command help to fail with
usage/env errors; reorder logic in the function so you first parse args into
command (client.CdCommand using args and the `args` slice) and handle
help/empty-command cases before checking required environment variables
(projectName and stackName) and returning usageError; apply the same reorder to
the similar block referenced around the second occurrence (lines 51-57) so help
is always honored prior to env validation.
- Around line 97-100: The call to auto.SelectStackInlineSource in the
client.CdCommandList branch is swallowing its error (stack, _ =
auto.SelectStackInlineSource(...)) and later code accesses stack.Workspace(),
which can panic if selection failed; change the branch to capture and check the
error from auto.SelectStackInlineSource(ctx, stackName, projectName, nil),
return or propagate the error immediately when non-nil (with context including
stackName/projectName), and only proceed to use stack.Workspace() when err ==
nil so downstream code is safe.
In `@cd/run.sh`:
- Around line 2-11: Remove the hardcoded Azure tenant/subscription/client
variables and stop unconditionally overriding caller-provided values: instead,
read AZURE_SUBSCRIPTION_ID, AZURE_LOCATION, AZURE_TENANT_ID, and AZURE_CLIENT_ID
from the environment (use them only if set) and provide safe defaults only for
non-sensitive vars like STACK, PROJECT, DEFANG_*; ensure the script fails fast
or errors if the required AZURE_* credentials are missing so callers must supply
valid account IDs rather than using baked-in values.
In `@cd/testdata/compose.cd.yaml`:
- Line 16: Replace the incorrect environment variable name
STATES_EVENTS_UPLOAD_URL with DEFANG_STATES_UPLOAD_URL in the compose CD YAML
(every occurrence at lines shown in the review), so the runtime code that reads
DEFANG_STATES_UPLOAD_URL will find the URL; update all instances (e.g.,
STATES_EVENTS_UPLOAD_URL -> DEFANG_STATES_UPLOAD_URL) to match the application's
expected variable name and ensure consistency with the already-correct
DEFANG_EVENTS_UPLOAD_URL.
In `@cd/testdata/main.go`:
- Around line 12-15: The code ignores the error from os.ReadFile before building
the ProjectUpdate; change the read to capture the error (e.g., compose, err :=
os.ReadFile(...)) and if err != nil handle it immediately (log/processLogger or
fmt.Fprintln to stderr and exit non‑zero) before calling
pulumi.Run(program.NewRun(&defangv1.ProjectUpdate{Compose: compose})); ensure
you return/exit on error so pulumi.Run is never invoked with an invalid/empty
compose payload.
In `@cd/testdata/preview-events-azure.json`:
- Around line 27-29: The diagnostic JSON comparison is failing because
developer-specific absolute plugin paths (e.g., the "using
pulumi-resource-defang-azure from $PATH at /Users/…/go/bin/…") aren't normalized
before the JSONEq assertion; update the test that runs the "cd preview"
diagnostics to add a regex normalization step (matching paths like "from \$PATH
at .*go/bin/[^\\s\"]+" or similar) that replaces the absolute plugin path in the
diagnostic "message" field with a stable placeholder, consistent with the
existing timestamp/sequence/random-suffix normalizers currently applied before
JSONEq so the golden comparison no longer depends on machine-specific install
locations.
In `@provider/defangazure/project.go`:
- Around line 388-410: Summary: Validate and normalize the LogConsoleLogsPlan
value before passing it to ARM to fail fast on typos. After retrieving
providerazure.LogConsoleLogsPlan via providerazure.LogConsoleLogsPlan.Get(ctx)
(consoleLogsPlan), resolve/convert it to a canonical string, normalize case,
check it is one of "Basic", "Analytics", or "Auxiliary", and if not return an
error (or stop creation) with a clear message; then pass the normalized value
into operationalinsights.TableArgs.Plan in the NewTable call (or use a validated
pulumi.StringOutput) so ARM never sees invalid plans.
---
Nitpick comments:
In @.claude/settings.json:
- Around line 7-21: The allowlist currently includes broad Azure CLI "list"
commands (e.g., "Bash(az account list:*)", "Bash(az group list:*)", "Bash(az
monitor log-analytics workspace table list:*)", "Bash(az role assignment
list:*)") which enable subscription-wide enumeration; replace those with the
narrowest resource-scoped or "show" variants (or remove them) so the assistant
only has read access to the specific integration resources under test—update the
listed entries (for example, keep "Bash(az account show:*)", "Bash(az group
show:*)", "Bash(az monitor log-analytics workspace show:*)", "Bash(az monitor
log-analytics workspace table show:*)", and remove or tighten the corresponding
"list" and "role assignment list" lines) to restrict scope.
In `@cd/run.sh`:
- Around line 4-5: The environment variables DEFANG_EVENTS_UPLOAD_URL and
STATES_EVENTS_UPLOAD_URL currently point to fixed files (/tmp/events-azure.json
and /tmp/states-azure.json) which will collide across parallel or repeated runs;
change the script to generate unique per-run temp file paths (e.g., via mktemp
or by including $$ and a timestamp) and assign those generated file paths to
DEFANG_EVENTS_UPLOAD_URL and STATES_EVENTS_UPLOAD_URL so each invocation writes
to its own artifact file.
🪄 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: 4007b7ab-f397-4196-8853-e9966dc50103
⛔ Files ignored due to path filters (2)
cd/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
.claude/settings.jsoncd/cd_main.gocd/cd_main_test.gocd/envs.gocd/envs_test.gocd/go.modcd/main.gocd/main_test.gocd/run.shcd/testdata/.defang/azurecd/testdata/Pulumi.azure.yamlcd/testdata/Pulumi.yamlcd/testdata/compose.cd.yamlcd/testdata/compose.crewai.yamlcd/testdata/main.gocd/testdata/preview-events-aws.jsoncd/testdata/preview-events-azure.jsoncd/testdata/preview-events-gcp.jsoncd/upload.gocd/upload_test.gogo.modprovider/defangazure/azure/acr.goprovider/defangazure/azure/dns.goprovider/defangazure/azure/image.goprovider/defangazure/azure/llm.goprovider/defangazure/azure/postgres.goprovider/defangazure/azure/recipe.goprovider/defangazure/azure/redis.goprovider/defangazure/project.go
💤 Files with no reviewable changes (3)
- provider/defangazure/azure/acr.go
- cd/envs.go
- provider/defangazure/azure/image.go
Comment on lines
+36
to
+46
| if projectName == "" { | ||
| return &usageError{msg: "missing required environment variable: PROJECT"} | ||
| } | ||
| if stackName != strings.ToLower(stackName) { | ||
| return &usageError{msg: "STACK name must be lowercase"} | ||
| } | ||
|
|
||
| var command client.CdCommand | ||
| if len(args) > 1 { | ||
| command = client.CdCommand(args[1]) | ||
| } |
Contributor
There was a problem hiding this comment.
Handle help/empty command before requiring PROJECT.
Right now Line 36 enforces PROJECT before command parsing, so cd help can fail with usage/env errors instead of showing help. Move command parsing/help handling ahead of required env validation.
Also applies to: 51-57
🤖 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 `@cd/cd_main.go` around lines 36 - 46, The code currently validates projectName
and stackName before parsing the command, causing `cd help` or empty-command
help to fail with usage/env errors; reorder logic in the function so you first
parse args into command (client.CdCommand using args and the `args` slice) and
handle help/empty-command cases before checking required environment variables
(projectName and stackName) and returning usageError; apply the same reorder to
the similar block referenced around the second occurrence (lines 51-57) so help
is always honored prior to env validation.
Comment on lines
+97
to
+100
| case client.CdCommandList: | ||
| // List doesn't need a real stack, but select something so we can call ListStacks on the workspace. | ||
| stack, _ = auto.SelectStackInlineSource(ctx, stackName, projectName, nil) | ||
| default: |
Contributor
There was a problem hiding this comment.
Don’t ignore stack selection errors in list flow.
Line 99 discards the error from auto.SelectStackInlineSource, but Line 240 then uses stack.Workspace(). If selection fails, this can break list unexpectedly. Handle and return the error explicitly.
Suggested fix
case client.CdCommandList:
// List doesn't need a real stack, but select something so we can call ListStacks on the workspace.
- stack, _ = auto.SelectStackInlineSource(ctx, stackName, projectName, nil)
+ stack, err = auto.SelectStackInlineSource(ctx, stackName, projectName, nil)
+ if err != nil {
+ return pulumiErr(err)
+ }Also applies to: 240-246
🤖 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 `@cd/cd_main.go` around lines 97 - 100, The call to
auto.SelectStackInlineSource in the client.CdCommandList branch is swallowing
its error (stack, _ = auto.SelectStackInlineSource(...)) and later code accesses
stack.Workspace(), which can panic if selection failed; change the branch to
capture and check the error from auto.SelectStackInlineSource(ctx, stackName,
projectName, nil), return or propagate the error immediately when non-nil (with
context including stackName/projectName), and only proceed to use
stack.Workspace() when err == nil so downstream code is safe.
Comment on lines
+2
to
+11
| export STACK=lioazure | ||
| export PROJECT=test | ||
| export DEFANG_EVENTS_UPLOAD_URL=file:///tmp/events-azure.json | ||
| export STATES_EVENTS_UPLOAD_URL=file:///tmp/states-azure.json | ||
| export DEFANG_PULUMI_DIFF=1 | ||
|
|
||
| export AZURE_SUBSCRIPTION_ID=f311c4db-e998-4c94-906c-7e2637303a05 | ||
| export AZURE_LOCATION=westus | ||
| export AZURE_TENANT_ID=12c0f515-fa47-402f-9982-de7646d3cb28 | ||
| export AZURE_CLIENT_ID=80fc261c-daa9-4d91-86f7-74c110dac086 |
Contributor
There was a problem hiding this comment.
Don’t bake a single Azure account into the shared runner.
This script commits environment-specific Azure identifiers and unconditionally overrides any caller-provided values, so it will fail for anyone outside that tenant/subscription and is awkward to reuse in CI. Require the account IDs from the environment and keep only safe defaults in the script.
Suggested change
#!/usr/bin/env bash
-export STACK=lioazure
-export PROJECT=test
-export DEFANG_EVENTS_UPLOAD_URL=file:///tmp/events-azure.json
-export STATES_EVENTS_UPLOAD_URL=file:///tmp/states-azure.json
-export DEFANG_PULUMI_DIFF=1
+set -euo pipefail
+
+export STACK="${STACK:-lioazure}"
+export PROJECT="${PROJECT:-test}"
+export DEFANG_EVENTS_UPLOAD_URL="${DEFANG_EVENTS_UPLOAD_URL:-file:///tmp/events-azure.json}"
+export STATES_EVENTS_UPLOAD_URL="${STATES_EVENTS_UPLOAD_URL:-file:///tmp/states-azure.json}"
+export DEFANG_PULUMI_DIFF="${DEFANG_PULUMI_DIFF:-1}"
-export AZURE_SUBSCRIPTION_ID=f311c4db-e998-4c94-906c-7e2637303a05
-export AZURE_LOCATION=westus
-export AZURE_TENANT_ID=12c0f515-fa47-402f-9982-de7646d3cb28
-export AZURE_CLIENT_ID=80fc261c-daa9-4d91-86f7-74c110dac086
+export AZURE_SUBSCRIPTION_ID="${AZURE_SUBSCRIPTION_ID:?set AZURE_SUBSCRIPTION_ID}"
+export AZURE_LOCATION="${AZURE_LOCATION:-westus}"
+export AZURE_TENANT_ID="${AZURE_TENANT_ID:?set AZURE_TENANT_ID}"
+export AZURE_CLIENT_ID="${AZURE_CLIENT_ID:?set AZURE_CLIENT_ID}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export STACK=lioazure | |
| export PROJECT=test | |
| export DEFANG_EVENTS_UPLOAD_URL=file:///tmp/events-azure.json | |
| export STATES_EVENTS_UPLOAD_URL=file:///tmp/states-azure.json | |
| export DEFANG_PULUMI_DIFF=1 | |
| export AZURE_SUBSCRIPTION_ID=f311c4db-e998-4c94-906c-7e2637303a05 | |
| export AZURE_LOCATION=westus | |
| export AZURE_TENANT_ID=12c0f515-fa47-402f-9982-de7646d3cb28 | |
| export AZURE_CLIENT_ID=80fc261c-daa9-4d91-86f7-74c110dac086 | |
| set -euo pipefail | |
| export STACK="${STACK:-lioazure}" | |
| export PROJECT="${PROJECT:-test}" | |
| export DEFANG_EVENTS_UPLOAD_URL="${DEFANG_EVENTS_UPLOAD_URL:-file:///tmp/events-azure.json}" | |
| export STATES_EVENTS_UPLOAD_URL="${STATES_EVENTS_UPLOAD_URL:-file:///tmp/states-azure.json}" | |
| export DEFANG_PULUMI_DIFF="${DEFANG_PULUMI_DIFF:-1}" | |
| export AZURE_SUBSCRIPTION_ID="${AZURE_SUBSCRIPTION_ID:?set AZURE_SUBSCRIPTION_ID}" | |
| export AZURE_LOCATION="${AZURE_LOCATION:-westus}" | |
| export AZURE_TENANT_ID="${AZURE_TENANT_ID:?set AZURE_TENANT_ID}" | |
| export AZURE_CLIENT_ID="${AZURE_CLIENT_ID:?set AZURE_CLIENT_ID}" |
🤖 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 `@cd/run.sh` around lines 2 - 11, Remove the hardcoded Azure
tenant/subscription/client variables and stop unconditionally overriding
caller-provided values: instead, read AZURE_SUBSCRIPTION_ID, AZURE_LOCATION,
AZURE_TENANT_ID, and AZURE_CLIENT_ID from the environment (use them only if set)
and provide safe defaults only for non-sensitive vars like STACK, PROJECT,
DEFANG_*; ensure the script fails fast or errors if the required AZURE_*
credentials are missing so callers must supply valid account IDs rather than
using baked-in values.
| - STACK=aws | ||
| - PROJECT=cd-test | ||
| - DEFANG_EVENTS_UPLOAD_URL=file:///tmp/defang-cd/events-aws.json | ||
| - STATES_EVENTS_UPLOAD_URL=file:///tmp/defang-cd/states-aws.json |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find env var names and state upload wiring used by CD code.
rg -n -C3 'STATES_EVENTS_UPLOAD_URL|STATE_EVENTS_UPLOAD_URL|DEFANG_EVENTS_UPLOAD_URL|uploadState|uploadEvents'Repository: DefangLabs/pulumi-defang
Length of output: 7247
🏁 Script executed:
rg -n 'statesUploadUrl' cd/cd_main.goRepository: DefangLabs/pulumi-defang
Length of output: 326
Fix env var name: STATES_EVENTS_UPLOAD_URL should be DEFANG_STATES_UPLOAD_URL.
Lines 16, 37, and 60 use STATES_EVENTS_UPLOAD_URL, but the CD code reads DEFANG_STATES_UPLOAD_URL (see cd/cd_main.go:34). This mismatch causes state uploads to silently no-op because the URL environment variable is not found. The events upload correctly uses DEFANG_EVENTS_UPLOAD_URL.
🤖 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 `@cd/testdata/compose.cd.yaml` at line 16, Replace the incorrect environment
variable name STATES_EVENTS_UPLOAD_URL with DEFANG_STATES_UPLOAD_URL in the
compose CD YAML (every occurrence at lines shown in the review), so the runtime
code that reads DEFANG_STATES_UPLOAD_URL will find the URL; update all instances
(e.g., STATES_EVENTS_UPLOAD_URL -> DEFANG_STATES_UPLOAD_URL) to match the
application's expected variable name and ensure consistency with the
already-correct DEFANG_EVENTS_UPLOAD_URL.
Comment on lines
+12
to
+15
| compose, _ := os.ReadFile("./compose.crewai.yaml") | ||
| pulumi.Run(program.NewRun(&defangv1.ProjectUpdate{ | ||
| Compose: compose, | ||
| })) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine its content
cat -n cd/testdata/main.goRepository: DefangLabs/pulumi-defang
Length of output: 527
🏁 Script executed:
# Check if make lint or golangci-lint is available
ls -la | grep -i makefile
cat Makefile | grep -A 5 "lint"Repository: DefangLabs/pulumi-defang
Length of output: 950
🏁 Script executed:
# Check for golangci-lint configuration
fd -t f '.golangci' -o '.golangci.yaml' -o '.golangci.yml'Repository: DefangLabs/pulumi-defang
Length of output: 216
🏁 Script executed:
# Look for golangci-lint config files with proper syntax
find . -maxdepth 2 -name ".golangci*" -type fRepository: DefangLabs/pulumi-defang
Length of output: 85
🏁 Script executed:
# Check if there are any lint-related configurations or if linting should cover examples/cd
grep -r "examples" Makefile | head -20Repository: DefangLabs/pulumi-defang
Length of output: 863
🏁 Script executed:
# Check if there's any documentation about where lint should be run
cat -n README.md | head -50Repository: DefangLabs/pulumi-defang
Length of output: 4383
🏁 Script executed:
# Verify the actual scope of the lint target and whether examples/cd should be linted
rg "examples/cd|testdata" --type goRepository: DefangLabs/pulumi-defang
Length of output: 299
🏁 Script executed:
# Check the exact linting scope and confirm cd/ is not included
cat -n .golangci.yaml | head -30Repository: DefangLabs/pulumi-defang
Length of output: 787
🏁 Script executed:
# Verify if there are any other lint configurations or pre-commit hooks that might cover cd/
cat -n .pre-commit-config.yaml 2>/dev/null || echo "No pre-commit config"Repository: DefangLabs/pulumi-defang
Length of output: 89
🏁 Script executed:
# Check if the cd/testdata directory has special status
ls -la cd/testdata/ | head -20Repository: DefangLabs/pulumi-defang
Length of output: 800
🏁 Script executed:
# Check the full golangci.yaml to see if cd/ is excluded
cat .golangci.yamlRepository: DefangLabs/pulumi-defang
Length of output: 2701
🏁 Script executed:
# Check the Makefile lint target scope again more carefully
cat Makefile | grep -A 10 "^lint:"Repository: DefangLabs/pulumi-defang
Length of output: 597
🏁 Script executed:
# Verify what the actual error handling issue is - check if blank import is intentional
# by looking at similar patterns in the codebase
rg "_, err :=" --type go | head -10Repository: DefangLabs/pulumi-defang
Length of output: 808
🏁 Script executed:
# Check if there's a specific pattern where errors are ignored in this codebase
rg "ReadFile\|os\.Open" --type go -A 2 | head -30Repository: DefangLabs/pulumi-defang
Length of output: 50
🏁 Script executed:
# Check if cd/testdata/main.go is actual test code or just data
cat -n cd/testdata/main.goRepository: DefangLabs/pulumi-defang
Length of output: 527
🏁 Script executed:
# Look for test files in cd/ to understand the testing structure
find cd -name "*_test.go" -type fRepository: DefangLabs/pulumi-defang
Length of output: 172
🏁 Script executed:
# Check if the cd directory has its own purpose/documentation
cat cd/README.md 2>/dev/null || echo "No README in cd/"Repository: DefangLabs/pulumi-defang
Length of output: 85
🏁 Script executed:
# Verify whether errcheck lint would catch this even if linting was run
# by checking the configuration settings for errcheck
rg "errcheck:" .golangci.yaml -A 10Repository: DefangLabs/pulumi-defang
Length of output: 50
Handle file read errors before constructing ProjectUpdate.
Line 12 ignores the os.ReadFile error, allowing a nil or empty compose payload to propagate into the Pulumi run. This can cause misleading runtime failures instead of failing fast at the source.
Suggested fix
func main() {
- compose, _ := os.ReadFile("./compose.crewai.yaml")
+ compose, err := os.ReadFile("./compose.crewai.yaml")
+ if err != nil {
+ panic(err)
+ }
pulumi.Run(program.NewRun(&defangv1.ProjectUpdate{
Compose: compose,
}))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| compose, _ := os.ReadFile("./compose.crewai.yaml") | |
| pulumi.Run(program.NewRun(&defangv1.ProjectUpdate{ | |
| Compose: compose, | |
| })) | |
| compose, err := os.ReadFile("./compose.crewai.yaml") | |
| if err != nil { | |
| panic(err) | |
| } | |
| pulumi.Run(program.NewRun(&defangv1.ProjectUpdate{ | |
| Compose: compose, | |
| })) |
🤖 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 `@cd/testdata/main.go` around lines 12 - 15, The code ignores the error from
os.ReadFile before building the ProjectUpdate; change the read to capture the
error (e.g., compose, err := os.ReadFile(...)) and if err != nil handle it
immediately (log/processLogger or fmt.Fprintln to stderr and exit non‑zero)
before calling pulumi.Run(program.NewRun(&defangv1.ProjectUpdate{Compose:
compose})); ensure you return/exit on error so pulumi.Run is never invoked with
an invalid/empty compose payload.
Comment on lines
+27
to
+29
| "prefix": "warning: ", | ||
| "message": "using pulumi-resource-defang-azure from $PATH at /Users/llunesu/go/bin/pulumi-resource-defang-azure\n", | ||
| "color": "never", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether test normalization handles absolute plugin path warnings.
rg -n -C3 'pulumi-resource-defang-azure|/Users/|warning:' cd/main_test.go cd/testdata/preview-events-azure.jsonRepository: DefangLabs/pulumi-defang
Length of output: 688
🏁 Script executed:
# Check if the test file exists and get its size
wc -l cd/main_test.go
# Search for comparison/assertion logic and normalization
rg -n 'expect|golden|normalize|fixture|assert|CompareString|diff' cd/main_test.go | head -30
# Look for the specific event or fixture being used
rg -n 'preview-events|diagnosticEvent' cd/main_test.goRepository: DefangLabs/pulumi-defang
Length of output: 556
🏁 Script executed:
# Read the test file with line numbers around the comparison
cat -n cd/main_test.go | sed -n '95,145p'Repository: DefangLabs/pulumi-defang
Length of output: 2308
🏁 Script executed:
# Search for the diagnostic message source
rg -n 'using pulumi-resource' . --type go
# Also check if there's any mocking or environment setup
rg -n 'pulumi-resource-defang|mock|Mock' cd/main_test.go | head -20Repository: DefangLabs/pulumi-defang
Length of output: 50
🏁 Script executed:
# Read the test function from the beginning
cat -n cd/main_test.go | sed -n '80,102p'Repository: DefangLabs/pulumi-defang
Length of output: 1225
🏁 Script executed:
# Read further back to find the Preview call and uploaded variable
cat -n cd/main_test.go | sed -n '50,85p'Repository: DefangLabs/pulumi-defang
Length of output: 1672
Normalize the absolute plugin path in diagnostic messages before golden comparison.
The test at line 72 executes a real cd preview command, which generates diagnostic output containing an absolute developer-specific path (/Users/llunesu/go/bin/pulumi-resource-defang-azure). While timestamps, sequences, and random suffixes are already normalized (lines 82–110), the diagnostic message string is not. This will cause JSONEq comparison to fail on any machine with a different plugin installation path. Add a regex normalization step (similar to the existing ones) to sanitize the plugin path from diagnostic messages before comparison.
🤖 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 `@cd/testdata/preview-events-azure.json` around lines 27 - 29, The diagnostic
JSON comparison is failing because developer-specific absolute plugin paths
(e.g., the "using pulumi-resource-defang-azure from $PATH at /Users/…/go/bin/…")
aren't normalized before the JSONEq assertion; update the test that runs the "cd
preview" diagnostics to add a regex normalization step (matching paths like
"from \$PATH at .*go/bin/[^\\s\"]+" or similar) that replaces the absolute
plugin path in the diagnostic "message" field with a stable placeholder,
consistent with the existing timestamp/sequence/random-suffix normalizers
currently applied before JSONEq so the golden comparison no longer depends on
machine-specific install locations.
Comment on lines
+388
to
+410
| // Pin the plan on the console logs table (Container Apps' stdout sink). | ||
| // The table schema isn't a true workspace built-in — it's registered by | ||
| // the Container Apps Monitor solution, which attaches when an env links | ||
| // the workspace as its log-analytics destination. So we must wait for | ||
| // `env` before the table is queryable (or its ID resolvable for Import). | ||
| // Plain Create would race with the solution registration and fail with | ||
| // "cannot create already existing resource"; Import + DependsOn(env) | ||
| // guarantees the schema exists, then applies the Plan diff. | ||
| consoleLogsPlan := providerazure.LogConsoleLogsPlan.Get(ctx) | ||
| consoleLogsImportID := logWorkspace.ID().ToStringOutput().ApplyT(func(id string) pulumi.ID { | ||
| return pulumi.ID(id + "/tables/" + consoleLogsTable) | ||
| }).(pulumi.IDOutput) | ||
| if _, err := operationalinsights.NewTable(ctx, name+"-console-logs", &operationalinsights.TableArgs{ | ||
| ResourceGroupName: infra.ResourceGroup.Name, | ||
| WorkspaceName: logWorkspace.Name, | ||
| TableName: pulumi.String(consoleLogsTable), | ||
| Plan: pulumi.String(consoleLogsPlan), | ||
| }, | ||
| parentOpt, | ||
| pulumi.Parent(logWorkspace), | ||
| pulumi.DependsOn([]pulumi.Resource{diagSetting}), | ||
| pulumi.Import(consoleLogsImportID), | ||
| ); err != nil { |
Contributor
There was a problem hiding this comment.
Validate LogConsoleLogsPlan before sending it to ARM.
providerazure.LogConsoleLogsPlan.Get(ctx) is passed straight into operationalinsights.Table.Plan, so a typo like basic or Basci becomes a late Azure deployment error instead of a clear config error. Normalize and reject anything outside Basic, Analytics, and Auxiliary before creating the table.
🤖 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 `@provider/defangazure/project.go` around lines 388 - 410, Summary: Validate
and normalize the LogConsoleLogsPlan value before passing it to ARM to fail fast
on typos. After retrieving providerazure.LogConsoleLogsPlan via
providerazure.LogConsoleLogsPlan.Get(ctx) (consoleLogsPlan), resolve/convert it
to a canonical string, normalize case, check it is one of "Basic", "Analytics",
or "Auxiliary", and if not return an error (or stop creation) with a clear
message; then pass the normalized value into operationalinsights.TableArgs.Plan
in the NewTable call (or use a validated pulumi.StringOutput) so ARM never sees
invalid plans.
2 tasks
2 tasks
lionello
force-pushed
the
lio/add-integ-test
branch
from
b0595a8 to
04833a8
Compare
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.
Basictier tablesSummary by CodeRabbit
New Features
Tests
Chores