Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

fix update-development-yaml will remove more webhooks than disabled one #3109

Merged
merged 1 commit into from
Jan 18, 2024
Merged

fix update-development-yaml will remove more webhooks than disabled one #3109

merged 1 commit into from
Jan 18, 2024

Conversation

lowang-bh
Copy link
Member

@lowang-bh lowang-bh commented Sep 9, 2023 •
edited
Loading

eg: when disable /pod/validate, then /queue/validate webook yaml also be removed. this pr fixed this issue

fixes #3129

The following pictures shows that extra webhook is removed before this PR.
image
image

@lowang-bh
fix update-development-yaml will remove more webhooks than disabled one
f9351f3 
eg: when disable /pod/validate, then /queue/validate webook yaml also be removed.
this pr fixed this issue

Signed-off-by: lowang-bh <lhui_wang@163.com>
@volcano-sh-bot volcano-sh-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 9, 2023
@lowang-bh
Copy link
Member Author

/assign @hwdef @wangyang0616 @william-wang

@Monokaix
Copy link
Member

what if we disable other webhook but not the second last pods/validation? Is it still not correct?

@lowang-bh
Copy link
Member Author

lowang-bh commented Sep 15, 2023 •
edited
Loading

what if we disable other webhook but not the second last pods/validation? Is it still not correct?

All case is ok now.

@Monokaix
Copy link
Member

Could you provide more details? I removed pods/validate in installer/helm/chart/volcano/values.yaml of enabled_admissions, it rendered correctly with helm template and other webhook is not removed.

@lowang-bh
Copy link
Member Author

lowang-bh commented Sep 15, 2023 •
edited
Loading

Could you provide more details? I removed pods/validate in installer/helm/chart/volcano/values.yaml of enabled_admissions, it rendered correctly with helm template and other webhook is not removed.

Just remove what you have disabled. eg: you removed pods/validate , then volcano-development.yaml should remove the pod validate webook, keep the others, like what you get now. Before this MR, there will be other webhooks also be removed. That is unexpected.

@lowang-bh lowang-bh mentioned this pull request Sep 16, 2023
Closed
@lowang-bh lowang-bh mentioned this pull request Oct 11, 2023
Closed
@Monokaix
Copy link
Member

helm template volcano was OK in my env，but helm template volcano -s webhook.yaml triggered the problem,seems like a helm bug.

@Monokaix
Copy link
Member

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2023
@lowang-bh
Copy link
Member Author

relative PR：#3153

william-wang
william-wang approved these changes Jan 18, 2024
View reviewed changes
Copy link
Member

@william-wang william-wang left a comment

Choose a reason for hiding this comment

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

/approve

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: william-wang

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@volcano-sh-bot volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2024
@volcano-sh-bot volcano-sh-bot merged commit 0b05bb7 into volcano-sh:master Jan 18, 2024
1 check passed
@lowang-bh lowang-bh deleted the helmGenYaml branch March 10, 2024 08:27
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@william-wang william-wang william-wang approved these changes

@jiangkaihua jiangkaihua Awaiting requested review from jiangkaihua

@shinytang6 shinytang6 Awaiting requested review from shinytang6

Assignees

@william-wang william-wang

@hwdef hwdef

@Monokaix Monokaix

@wangyang0616 wangyang0616

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

make update-development-yaml will remove more webhooks than disabled one
6 participants
@lowang-bh @Monokaix @volcano-sh-bot @william-wang @hwdef @wangyang0616