[Feature] Refactor namespace provisioning and simplify default tenant space module - #258
[Feature] Refactor namespace provisioning and simplify default tenant space module#258sathindudezoysa wants to merge 3 commits into
Conversation
…efault tenant space module
|
Warning Review limit reached
Next review available in: 21 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a configurable Rancher namespace module with metadata, cluster-wait settings, resource limits, quotas, validation, and outputs. Updates tenant spaces to create and expose one conditional default namespace instead of multiple configured namespaces. ChangesTenant namespace management
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/tenancy/tenant-space/main.tf (1)
60-64: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove deleted inputs from the precondition message.
If
cpu_limitis null and another quota value is set, the error tells users to removenamespace_*_limitinputs. This module no longer accepts these inputs. Name onlymemory_limitandstorage_limit.Proposed fix
- error_message = "Quota variables (memory_limit, storage_limit, namespace_*_limit) are only applied when cpu_limit is set. Either set cpu_limit or remove the other quota variables." + error_message = "Quota variables (memory_limit, storage_limit) are only applied when cpu_limit is set. Either set cpu_limit or remove the other quota variables."🤖 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 `@modules/tenancy/tenant-space/main.tf` around lines 60 - 64, Update the error_message in the quota validation precondition to mention only memory_limit and storage_limit, removing the obsolete namespace_*_limit reference while preserving the existing validation condition.
🤖 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 `@modules/tenancy/tenant-namespace/variables.tf`:
- Around line 32-36: Update the validation condition for var.namespaces in the
validation block to require each namespace key to match the existing RFC 1123
character pattern and have a maximum length of 63 characters. Preserve the
current error message and all existing start/end character rules.
In `@modules/tenancy/tenant-space/main.tf`:
- Around line 91-94: Update the namespace resource declaration around
local.namespace to preserve the previous for_each-based management of every
configured namespace instead of reducing it to count[0]. Keep existing keyed
resource instances addressable and retain namespace_ids as a map-compatible
output; if changing resource addressing is unavoidable, add explicit state
migration and handling for all supported namespace configurations.
---
Outside diff comments:
In `@modules/tenancy/tenant-space/main.tf`:
- Around line 60-64: Update the error_message in the quota validation
precondition to mention only memory_limit and storage_limit, removing the
obsolete namespace_*_limit reference while preserving the existing validation
condition.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 731c9204-1aa3-4fef-ba1d-e45a5cc0fd4d
📒 Files selected for processing (7)
modules/tenancy/tenant-namespace/main.tfmodules/tenancy/tenant-namespace/outputs.tfmodules/tenancy/tenant-namespace/variables.tfmodules/tenancy/tenant-namespace/versions.tfmodules/tenancy/tenant-space/main.tfmodules/tenancy/tenant-space/outputs.tfmodules/tenancy/tenant-space/variables.tf
💤 Files with no reviewable changes (1)
- modules/tenancy/tenant-space/variables.tf
| #check for null to avoid creating a namespace when create_default_namespace = false | ||
| count = local.namespace != null ? 1 : 0 | ||
|
|
||
| name = local.namespace |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect existing in-module refactoring support and resource instance declarations.
rg -n -C 4 --glob '*.tf' \
'^\s*(moved|removed)\s*\{|rancher2_namespace\s+"this"|for_each\s*=|count\s*=' \
modules/tenancy/tenant-spaceRepository: wso2/open-cloud-datacenter
Length of output: 10098
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- targeted diff ---'
git diff -- modules/tenancy/tenant-space/main.tf
printf '%s\n' '--- current resource and locals ---'
sed -n '1,115p' modules/tenancy/tenant-space/main.tf
printf '%s\n' '--- refactoring declarations ---'
rg -n -C 3 --glob '*.tf' '^\s*(moved|removed)\s*\{' .
printf '%s\n' '--- namespace resource references ---'
rg -n --glob '*.tf' 'rancher2_namespace\.(this|network|common)|tenant-space' .Repository: wso2/open-cloud-datacenter
Length of output: 10761
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- recent file history ---'
git log -n 8 --oneline -- modules/tenancy/tenant-space/main.tf
printf '%s\n' '--- parent resource declaration ---'
parent="$(git rev-parse HEAD^)"
git show "$parent:modules/tenancy/tenant-space/main.tf" 2>/dev/null |
nl -ba | sed -n '75,115p' || true
printf '%s\n' '--- current and parent namespace-related outputs ---'
for rev in HEAD "$parent"; do
printf '%s\n' "--- $rev ---"
git show "$rev:modules/tenancy/tenant-space/outputs.tf" 2>/dev/null |
nl -ba | sed -n '1,45p' || true
done
printf '%s\n' '--- current revision ---'
git rev-parse --short HEADRepository: wso2/open-cloud-datacenter
Length of output: 941
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
parent="$(git rev-parse HEAD^)"
printf '%s\n' '--- parent resource declaration ---'
git show "$parent:modules/tenancy/tenant-space/main.tf" 2>/dev/null |
sed -n '75,115p' || true
printf '%s\n' '--- current resource declaration ---'
sed -n '85,105p' modules/tenancy/tenant-space/main.tf
printf '%s\n' '--- current and parent outputs ---'
for rev in HEAD "$parent"; do
printf '%s\n' "--- $rev ---"
git show "$rev:modules/tenancy/tenant-space/outputs.tf" 2>/dev/null |
sed -n '1,45p' || true
done
printf '%s\n' '--- parent commit summary ---'
git show --stat --oneline --no-renames "$parent" -- modules/tenancy/tenant-space/main.tfRepository: wso2/open-cloud-datacenter
Length of output: 7057
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
parent="$(git rev-parse HEAD^)"
printf '%s\n' '--- parent namespace inputs and local.namespaces ---'
git show "$parent:modules/tenancy/tenant-space/main.tf" 2>/dev/null |
sed -n '1,85p' || true
git show "$parent:modules/tenancy/tenant-space/variables.tf" 2>/dev/null |
sed -n '1,120p' || true
printf '%s\n' '--- namespace_ids consumers ---'
rg -n -C 3 --glob '*.tf' 'namespace_ids' .
printf '%s\n' '--- tenant-space module calls and namespace outputs ---'
rg -n -C 4 --glob '*.tf' 'source\s*=.*tenant-space|module\.[A-Za-z0-9_-]+\.namespace_ids|output\s+"namespace_ids"' .Repository: wso2/open-cloud-datacenter
Length of output: 13431
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import re
import subprocess
path = "modules/tenancy/tenant-space/main.tf"
parent = subprocess.check_output(["git", "rev-parse", "HEAD^"], text=True).strip()
old = subprocess.check_output(["git", "show", f"{parent}:{path}"], text=True)
new = open(path, encoding="utf-8").read()
def resource_body(text):
match = re.search(
r'resource\s+"rancher2_namespace"\s+"this"\s*\{(.*?)(?=\n\})',
text,
re.S,
)
if not match:
raise SystemExit("namespace resource not found")
return match.group(1)
old_body = resource_body(old)
new_body = resource_body(new)
assert 'for_each = toset(local.namespaces)' in old_body
assert re.search(r'\bcount\s*=\s*local\.namespace\s*!=\s*null\s*\?\s*1\s*:\s*0', new_body)
assert 'name = each.value' in old_body
assert 'name = local.namespace' in new_body
old_local = re.search(
r'namespaces\s*=\s*(.*)',
old,
).group(1)
assert 'var.namespaces' in old_local
old_output = subprocess.check_output(
["git", "show", f"{parent}:modules/tenancy/tenant-space/outputs.tf"],
text=True,
)
new_output = open("modules/tenancy/tenant-space/outputs.tf", encoding="utf-8").read()
assert 'value = { for ns, r in rancher2_namespace.this' in old_output
assert 'value = one(rancher2_namespace.this[*].id)' in new_output
print("old resource address shape: rancher2_namespace.this[<namespace>]")
print("new resource address shape: rancher2_namespace.this[0] when enabled")
print("old configuration supports multiple var.namespaces entries")
print("old namespace_ids output: map; new namespace_ids output: single ID")
print("mapping result: keyed existing instances cannot automatically map to count[0]")
PYRepository: wso2/open-cloud-datacenter
Length of output: 501
Preserve existing namespace state and output compatibility.
The previous resource used for_each over var.namespaces, but the new resource manages only count[0]. Existing keyed instances cannot map automatically to count[0]; additional namespaces can be destroyed, and namespace_ids changes from a map to a single ID. Preserve for_each, or provide explicit migration and handling for every supported namespace configuration.
🤖 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 `@modules/tenancy/tenant-space/main.tf` around lines 91 - 94, Update the
namespace resource declaration around local.namespace to preserve the previous
for_each-based management of every configured namespace instead of reducing it
to count[0]. Keep existing keyed resource instances addressable and retain
namespace_ids as a map-compatible output; if changing resource addressing is
unavoidable, add explicit state migration and handling for all supported
namespace configurations.
There was a problem hiding this comment.
We acknowledge that moving from for_each to count and changing the namespace_ids output is a breaking change for existing states. However, we are intentionally accepting this trade-off.
There was a problem hiding this comment.
@sathindudezoysa, acknowledged. This PR intentionally breaks existing Terraform resource addresses and the namespace_ids output contract. The migration impact remains for existing consumers, but it is an accepted trade-off.
🐇
✏️ Learnings added
Learnt from: sathindudezoysa
Repo: wso2/open-cloud-datacenter PR: 258
File: modules/tenancy/tenant-space/main.tf:91-94
Timestamp: 2026-08-12T12:42:29.327Z
Learning: In `modules/tenancy/tenant-space`, the intentional migration from `for_each` to `count` for `rancher2_namespace.this` and the change of `namespace_ids` from a map to a scalar are accepted breaking changes for existing Terraform states and consumers.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
1 participant
Summary
This PR extracts the namespace creation logic out of the tenant-space module into new, standalone tenant-namespace modules. This refactor solves the issue of rigid, project-wide default namespace limits by allowing users to define specific container resource limits and resource quotas on a per-namespace basis. The tenant-space module is now simplified to only handle the creation of the Rancher project and a single optional default namespace.
closes #236 #243
Changes
modules/tenancy/tenant-namespace: Added a module to create namespaces from a map variable. This allows for distinct, per-namespace container resource limits and resource quotas.modules/tenancy/tenant-space: Removed thenamespaceslist variable. Removednamespace_cpu_limit,namespace_memory_limit, andnamespace_storage_limit variables.modules/tenancy/tenant-space: Changed the namespace_ids output from a map of namespace IDs to a single string outputting the default namespace ID.Testing
Executed terraform apply to verify that existing tenant-space resources transition smoothly and new tenant-namespace resources are planned correctly with their respective limits and quotas in the LK-DC dev environment
Checklist
Summary by CodeRabbit