Skip to content

Add crossplane-namespace tag to managed resources#876

Merged
bobh66 merged 1 commit intocrossplane:mainfrom
Scrumplex:feat/add-crossplane-namespace-tag
Oct 16, 2025
Merged

Add crossplane-namespace tag to managed resources#876
bobh66 merged 1 commit intocrossplane:mainfrom
Scrumplex:feat/add-crossplane-namespace-tag

Conversation

@Scrumplex
Copy link
Contributor

Description of your changes

Since Crossplane v2 supports namespaced resources, it is quite fitting to also
mention the namespace of a resource in its tags.

I have:

Need help with this checklist? See the cheat sheet.

Since Crossplane v2 supports namespaced resources, it is quite fitting
to also mention the namespace of a resource in its tags.

Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
@Scrumplex Scrumplex requested a review from a team as a code owner October 13, 2025 21:04
@Scrumplex Scrumplex requested a review from turkenh October 13, 2025 21:04
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 13, 2025

📝 Walkthrough

Walkthrough

Adds a new exported tag key for namespace and updates GetExternalTags to include the object’s namespace when present. Introduces a corresponding test case verifying the namespace tag is populated for namespaced objects.

Changes

Cohort / File(s) Summary
Tagging logic
pkg/resource/resource.go
Adds ExternalResourceTagKeyNamespace constant and updates GetExternalTags to set the namespace tag when mg.GetNamespace() is non-empty.
Unit tests
pkg/resource/resource_test.go
Adds "SuccessfulWithNamespacedObject" test case to assert inclusion of the namespace tag in external tags for namespaced objects.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Thanks for the update. One quick clarification: should cluster-scoped objects explicitly omit the namespace tag, or include it with an empty value for consistency?

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title accurately reflects the main change by specifying that a crossplane-namespace tag is being added to managed resources and meets the under-72-character requirement.
Description Check ✅ Passed The description clearly outlines the rationale for adding a namespace tag to managed resources, mentions the updated tests, and directly relates to the code changes.
Breaking Changes ✅ Passed Thanks for adding the namespace tagging—it helps explain the motivation nicely. I reviewed the public API surface and the only change is the introduction of the new exported constant ExternalResourceTagKeyNamespace, while existing exported functions, types, methods, and fields retain their names, signatures, and behavior. Since no public elements were removed or altered incompatibly, the Breaking Changes check passes without needing the label.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3f03019 and 89bbbbb.

📒 Files selected for processing (2)
  • pkg/resource/resource.go (2 hunks)
  • pkg/resource/resource_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go

⚙️ CodeRabbit configuration file

**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.

Files:

  • pkg/resource/resource.go
  • pkg/resource/resource_test.go
**/*_test.go

⚙️ CodeRabbit configuration file

**/*_test.go: Enforce table-driven test structure: PascalCase test names (no
underscores), args/want pattern, use cmp.Diff with
cmpopts.EquateErrors() for error testing. Check for proper test case
naming and reason fields. Ensure no third-party test frameworks (no
Ginkgo, Gomega, Testify).

Files:

  • pkg/resource/resource_test.go
🧬 Code graph analysis (1)
pkg/resource/resource_test.go (3)
pkg/resource/interfaces.go (3)
  • ModernManaged (209-213)
  • TypedProviderConfigReferencer (84-87)
  • Managed (200-204)
pkg/resource/fake/mocks.go (3)
  • ModernManaged (370-376)
  • TypedProviderConfigReferencer (71-71)
  • Managed (344-348)
pkg/resource/resource.go (5)
  • ExternalResourceTagKeyKind (50-50)
  • ExternalResourceTagKeyName (51-51)
  • ExternalResourceTagKeyNamespace (52-52)
  • ExternalResourceTagKeyProvider (53-53)
  • ExternalResourceTagKeyProviderConfigKind (54-54)
🔇 Additional comments (3)
pkg/resource/resource.go (2)

52-52: LGTM! Constant naming is consistent.

The new constant follows the established naming pattern for external resource tag keys.


418-420: Nice implementation using the declare-in-conditional pattern.

The code correctly:

  • Declares the namespace variable in the conditional scope (as per coding guidelines)
  • Only adds the tag when namespace is non-empty
  • Maintains backward compatibility for cluster-scoped resources

This aligns well with Crossplane v2's support for namespaced resources.

pkg/resource/resource_test.go (1)

639-656: Excellent test coverage for the namespace functionality.

The test case properly verifies that:

  • The namespace tag is included when the managed resource has a namespace
  • All other tags are still populated correctly
  • Follows the established table-driven test pattern with PascalCase naming

The existing test cases (like "SuccessfulWithTypedProviderConfig") already cover the scenario where namespace is empty, ensuring backward compatibility.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me!

@bobh66 bobh66 merged commit 0ff38a9 into crossplane:main Oct 16, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants