search

karmada-io / karmada

Open, Multi-Cloud, Multi-Cluster Kubernetes Orchestration
https://karmada.io
Apache License 2.0
4.12k stars 806 forks source link

add validation for cluster api and remove validating webhook #1152

Closed carlory closed 2 years ago

carlory commented 2 years ago

What type of PR is this?

/kind feature

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

add validation for cluster API
karmada-bot commented 2 years ago

Welcome @carlory! It looks like this is your first PR to karmada-io/karmada 🎉

XiShanYongYe-Chang commented 2 years ago

Hi @carlory , thanks for your contribution. :)

carlory commented 2 years ago

/cc @kevin-wangzefeng @RainbowMango

RainbowMango commented 2 years ago

Thanks @carlory I'll review it ASAP.

carlory commented 2 years ago

/cc @kevin-wangzefeng @RainbowMango

RainbowMango commented 2 years ago

Got it @carlory.

carlory commented 2 years ago

add validation for impersonatorSecretRef /cc @RainbowMango @XiShanYongYe-Chang

carlory commented 2 years ago

/cc @RainbowMango @XiShanYongYe-Chang

remove validating webhook

XiShanYongYe-Chang commented 2 years ago

Hi @carlory, I'll review it now.

XiShanYongYe-Chang commented 2 years ago

Thanks, @carlory. It looks good to me.

RainbowMango commented 2 years ago

Not an objection. Just a question.

The validation functionality in webhook still works as expected, right? If it is the case, I'd suggest doing this after the coming new release.

RainbowMango commented 2 years ago

We should remove the configuration from karmada init: https://github.com/karmada-io/karmada/blob/14487548dd9c4970dad003da7d253c25811f2515/pkg/karmadactl/cmdinit/karmada/webhook_configuration.go#L83-L98

cc @lonelyCZ @prodanlabs

lonelyCZ commented 2 years ago

We should remove the configuration from karmada init

Ok, I will do it.

lonelyCZ commented 2 years ago

This can be removed in this pr. Thanks @carlory

carlory commented 2 years ago

/cc @RainbowMango

the validation in pkg/apis/cluster/validation is more complete than in pkg/util/validation.

In my opinion, there is no need to keep two verification processes at the same time

carlory commented 2 years ago

/cc @lonelyCZ I have removed the configuration from karmada init in this pr

carlory commented 2 years ago

/cc @RainbowMango @XiShanYongYe-Chang

XiShanYongYe-Chang commented 2 years ago

thanks! /lgtm

RainbowMango commented 2 years ago

/assign

RainbowMango commented 2 years ago

@carlory Please cc me again if it is ready for review. Thanks for doing this, appreciate it!!

carlory commented 2 years ago

/cc @RainbowMango @XiShanYongYe-Chang

It's ready for review.

carlory commented 2 years ago

@XiShanYongYe-Chang done.

karmada-bot commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/karmada-io/karmada/blob/master/OWNERS)~~ [RainbowMango] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
XiShanYongYe-Chang commented 2 years ago

/lgtm