Conversation
rauneetverma
requested review from
anshulsao,
ishaankalra,
pramodh-ayyappan,
rr0hit,
unni-facets and
vishnukv-facets
as code owners
WalkthroughThe change adds a new label Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/artifactories/default/0.1/registry_secret.tf`:
- Line 6: The merge call for the labels in registry_secret.tf can fail when
local.metadata.labels exists but is null; wrap the lookup(local.metadata,
"labels", {}) with a null-coalescing guard (e.g., coalesce(..., {})) so merge
always receives a map, and apply the same pattern to the labels for-loop in
locals.tf (the comprehension that builds the joined labels string) by coalescing
the lookup result to an empty map before iterating.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: baf01528-1120-4f48-89bf-181f5916826d
📒 Files selected for processing (1)
modules/artifactories/default/0.1/registry_secret.tf
| name = each.value.name | ||
| namespace = local.namespace | ||
| labels = lookup(local.metadata, "labels", {}) | ||
| labels = merge(lookup(local.metadata, "labels", {}), {"secret-copier" = "yes"}) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find explicit null label assignments that would break merge()
rg -n -C2 '\blabels\s*=\s*null\b|"labels"\s*:\s*null'
# 2) Inspect metadata/labels shapes in Terraform/YAML/JSON inputs
rg -n -C2 '\bmetadata\b|\blabels\b' -g '*.tf' -g '*.tfvars' -g '*.yaml' -g '*.yml' -g '*.json'
# 3) Locate this module's variable definitions to confirm expected type of metadata.labels
fd 'variables.tf$|locals.tf$|README.md$' modules | xargs -r rg -n -C2 '\bvariable\b|\bmetadata\b|\blabels\b'Repository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# View the file in question
cat -n modules/artifactories/default/0.1/registry_secret.tfRepository: Facets-cloud/facets-modules
Length of output: 565
🏁 Script executed:
# View locals.tf to understand local.metadata
cat -n modules/artifactories/default/0.1/locals.tfRepository: Facets-cloud/facets-modules
Length of output: 3799
🏁 Script executed:
# View variables.tf for metadata variable definition
cat -n modules/artifactories/default/0.1/variables.tfRepository: Facets-cloud/facets-modules
Length of output: 1241
🏁 Script executed:
# Check for example inputs or README
fd 'README|example|test' modules/artifactories/default/0.1 -type fRepository: Facets-cloud/facets-modules
Length of output: 241
🏁 Script executed:
# Check if there's a root level README with usage info
fd 'README' modules/artifactories -type fRepository: Facets-cloud/facets-modules
Length of output: 241
🏁 Script executed:
find modules/artifactories/default/0.1 -name "*.md" -o -name "*.tf" -o -name "*.json" | head -20Repository: Facets-cloud/facets-modules
Length of output: 360
🏁 Script executed:
# Check for test directory or examples
fd 'test\|example' modules/artifactories --maxdepth 3Repository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# Look for any validation or conditional handling around metadata.labels
rg -n 'metadata\.labels|type.*labels' modules/artifactories/default/0.1/Repository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# Check if there's a schema or docs describing expected input structure
cat modules/artifactories/default/0.1/README.md 2>/dev/null || echo "No README found"Repository: Facets-cloud/facets-modules
Length of output: 2347
Harden merge() and loop inputs to prevent failures on null labels.
Line 6 breaks if metadata.labels is present but null, because merge() only accepts maps/objects. In Terraform, lookup(key, default) returns the actual value if the key exists—even if that value is null—so it does not fall back to the default {}. The same vulnerability exists at locals.tf line 62 in the for-loop.
Suggested fix for registry_secret.tf line 6
- labels = merge(lookup(local.metadata, "labels", {}), {"secret-copier" = "yes"})
+ labels = merge(try(lookup(local.metadata, "labels", {})), {}), {"secret-copier" = "yes"})Also apply a similar defensive pattern to locals.tf line 62:
labels = join(",", [for k, v in lookup(local.metadata, "labels", {}) : "${k}=${v}"])
📝 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.
| labels = merge(lookup(local.metadata, "labels", {}), {"secret-copier" = "yes"}) | |
| labels = merge(try(lookup(local.metadata, "labels", {}), {}), {"secret-copier" = "yes"}) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/artifactories/default/0.1/registry_secret.tf` at line 6, The merge
call for the labels in registry_secret.tf can fail when local.metadata.labels
exists but is null; wrap the lookup(local.metadata, "labels", {}) with a
null-coalescing guard (e.g., coalesce(..., {})) so merge always receives a map,
and apply the same pattern to the labels for-loop in locals.tf (the
comprehension that builds the joined labels string) by coalescing the lookup
result to an empty map before iterating.
1 participant
Description
the artifactories module only uses the labels from metadata.labels but it doesn't automatically add the secret-copier: yes label.
When we created the artifactories resource in the blueprint the secret was created without secret-copier yes labels hence the secret copier which is running was not able to copy the secret in the new ns.
Related issues
Type of change
Checklist
developbranchTesting
Reviewer instructions
Summary by CodeRabbit