Skip to content

Conversation

@Arvindthiru
Copy link
Contributor

@Arvindthiru Arvindthiru commented Jun 22, 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: Add label to role, rolebindings & namespace feat: Add label to role, rolebinding & namespace Jun 22, 2023
@Arvindthiru Arvindthiru marked this pull request as draft June 22, 2023 20:22
@Arvindthiru Arvindthiru marked this pull request as ready for review June 23, 2023 21:03
@Arvindthiru Arvindthiru marked this pull request as draft June 23, 2023 23:01
@Arvindthiru
Copy link
Contributor Author

Arvindthiru commented Jun 23, 2023

Misunderstood the NamespaceSelector in ValidatingWebhook Object it selects the object that our webhook should handle based on the object's namespace and labels on the namespace, so no need to add labels on Role and Rolebindings we create

@Arvindthiru Arvindthiru marked this pull request as ready for review June 26, 2023 16:38
@ryanzhang-oss
Copy link
Contributor

How do we select Role and RoleBindings then?

Misunderstood the NamespaceSelector in ValidatingWebhook Object it selects the object that our webhook should handle based on the object's namespace and labels on the namespace, so no need to add labels on Role and Rolebindings we create

@ryanzhang-oss
Copy link
Contributor

I just realized that we have new version of API but the webhooks only works for v1alpha1. How should we handle that?

@Arvindthiru
Copy link
Contributor Author

Arvindthiru commented Jun 26, 2023

How do we select Role and RoleBindings then?

Misunderstood the NamespaceSelector in ValidatingWebhook Object it selects the object that our webhook should handle based on the object's namespace and labels on the namespace, so no need to add labels on Role and Rolebindings we create

Since we add the label on the namespace, we can specify the validating webhook such that it only handle roles and rolebindings within the namespace with the label we added

@Arvindthiru
Copy link
Contributor Author

I just realized that we have new version of API but the webhooks only works for v1alpha1. How should we handle that?

We can do something like this #397

@Arvindthiru Arvindthiru marked this pull request as draft June 28, 2023 23:26
@Arvindthiru Arvindthiru marked this pull request as ready for review June 29, 2023 21:01
@Arvindthiru Arvindthiru changed the title feat: Add label to role, rolebinding & namespace feat: Add label to fleet namespace Jun 29, 2023
@Arvindthiru Arvindthiru changed the title feat: Add label to fleet namespace feat: Add fleet resource label to fleet namespace Jun 29, 2023
@Arvindthiru Arvindthiru force-pushed the addFleetLabel branch 2 times, most recently from 40be576 to cbdec5b Compare July 4, 2023 16:52
michaelawyu
michaelawyu previously approved these changes Jul 5, 2023
Copy link
Contributor

@michaelawyu michaelawyu left a comment

Choose a reason for hiding this comment

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

Aside from the nit everything looks great ✨

@ryanzhang-oss ryanzhang-oss merged commit b6af7aa into Azure:main Jul 5, 2023
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