Skip to content

feat: v1beta1 CRP validation for PickAll Placement policy#552

Merged
ryanzhang-oss merged 6 commits intoAzure:mainfrom
Arvindthiru:v1beta1CRP2
Oct 11, 2023
Merged

feat: v1beta1 CRP validation for PickAll Placement policy#552
ryanzhang-oss merged 6 commits intoAzure:mainfrom
Arvindthiru:v1beta1CRP2

Conversation

@Arvindthiru
Copy link
Copy Markdown
Contributor

@Arvindthiru Arvindthiru commented Oct 5, 2023

Description of your changes

Fixes #

I have:

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

How has this code been tested

Special notes for your reviewer

@Arvindthiru Arvindthiru changed the title feat: validate placment policy type PickAll feat: v1beta1 CRP validate placment policy type PickAll Oct 5, 2023
@Arvindthiru Arvindthiru force-pushed the v1beta1CRP2 branch 2 times, most recently from f368813 to 64b0b1f Compare October 10, 2023 20:37
@Arvindthiru Arvindthiru marked this pull request as ready for review October 10, 2023 20:39
@Arvindthiru Arvindthiru changed the title feat: v1beta1 CRP validate placment policy type PickAll feat: v1beta1 CRP validation for PickAll Placement policy Oct 10, 2023
Comment thread pkg/utils/validator/clusterresourceplacement_test.go Outdated
Comment thread test/e2e/webhook_test.go
}, testutils.PollTimeout, testutils.PollInterval).Should(Succeed())
})

It("should deny update on CRP with invalid placement policy for PickAll", func() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Strictly speaking we don't really need this e2e. Doesn't hurt to have one but why not pick something specific error related to the pickAll?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, we don't actually need the E2Es but I wanted to at least cover some important cases in E2Es that are already covered by UTs just to provide solid proof

I am testing a bad label selector within cluster affinity which is within RequiredDuringSchedulingIgnoredDuringExecution which is specific to PickAll, I also added topology constraints which is not allowed for PickAll

Comment thread pkg/utils/validator/clusterresourceplacement_test.go
}
}

func Test_ValidateClusterResourcePlacement_PickAllPlacementPolicy(t *testing.T) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is the invalid case of PreferreAffinity covered?

@ryanzhang-oss ryanzhang-oss merged commit f451d30 into Azure:main Oct 11, 2023
britaniar added a commit to britaniar/fleet that referenced this pull request Mar 26, 2026
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.

2 participants