Skip to content

Conversation

@weng271190436
Copy link
Collaborator

Description of your changes

Fixes #311

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

@codecov
Copy link

codecov bot commented Oct 31, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@weng271190436 weng271190436 requested a review from Copilot November 3, 2025 15:43
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors validation rules for ClusterResourcePlacement and ResourcePlacement CRDs by separating the immutability checks for the policy field and the placement type into two distinct validation rules. Previously, a single validation rule checked both conditions, which has been split for better clarity and maintainability.

  • Separated validation for policy removal and placement type changes into two distinct rules
  • Added nested placement type immutability validation on the PlacementPolicy object
  • Updated test expectations to match the new error messages
  • Added comprehensive integration tests for ResourcePlacement validation scenarios

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
apis/placement/v1beta1/clusterresourceplacement_types.go Updated kubebuilder validation annotations to split policy removal check from placement type immutability check; added placement type immutability validation on PlacementSpec.Policy field
config/crd/bases/placement.kubernetes-fleet.io_clusterresourceplacements.yaml Applied generated CRD changes with separate validation rules for policy removal and placement type immutability
config/crd/bases/placement.kubernetes-fleet.io_resourceplacements.yaml Applied generated CRD changes for ResourcePlacement CRD with the same validation pattern as ClusterResourcePlacement
test/apis/placement/v1beta1/api_validation_integration_test.go Updated test error message expectations and added new test context for ResourcePlacement validation covering nil policy and placement type change scenarios

Wei Weng added 5 commits November 3, 2025 15:48
Signed-off-by: Wei Weng <Wei.Weng@microsoft.com>
Signed-off-by: Wei Weng <Wei.Weng@microsoft.com>
Signed-off-by: Wei Weng <Wei.Weng@microsoft.com>
Signed-off-by: Wei Weng <Wei.Weng@microsoft.com>
Signed-off-by: Wei Weng <Wei.Weng@microsoft.com>
@ryanzhang-oss ryanzhang-oss merged commit 2fd4460 into kubefleet-dev:main Nov 3, 2025
23 of 25 checks passed
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

// Policy defines how to select member clusters to place the selected resources.
// If unspecified, all the joined member clusters are selected.
// +kubebuilder:validation:Optional
// +kubebuilder:validation:XValidation:rule="!(self.placementType != oldSelf.placementType)",message="placement type is immutable"
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

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

This validation rule will fail when Policy is initially set from nil to a non-nil value because oldSelf will be nil and accessing oldSelf.placementType will cause an error. The rule should check if oldSelf exists first: rule=\"!has(oldSelf) || !(self.placementType != oldSelf.placementType)\"

Suggested change
// +kubebuilder:validation:XValidation:rule="!(self.placementType != oldSelf.placementType)",message="placement type is immutable"
// +kubebuilder:validation:XValidation:rule="!has(oldSelf) || !(self.placementType != oldSelf.placementType)",message="placement type is immutable"

Copilot uses AI. Check for mistakes.
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.

[BUG] RP allows bad creation and update which the controller can't handle

2 participants