Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Allow vpa-admission-controler to installl on specific path #1625

Merged
merged 1 commit into from
Feb 27, 2019
Merged

Allow vpa-admission-controler to installl on specific path #1625

merged 1 commit into from
Feb 27, 2019

Conversation

Rajat-0
Copy link
Contributor

@Rajat-0 Rajat-0 commented Jan 30, 2019 •
edited
Loading

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 30, 2019
@bskiba
Copy link
Member

bskiba commented Jan 30, 2019

Can you reference the original issue that this is addressing?

bskiba
bskiba suggested changes Feb 1, 2019
View reviewed changes
Copy link
Member

@bskiba bskiba left a comment

Choose a reason for hiding this comment

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

Overall I think we should do this but I would split into two flags.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 14, 2019
@Rajat-0 Rajat-0 changed the title [WIP]Allow vpa-admission-controler to installl on specific path Allow vpa-admission-controler to installl on specific path Feb 14, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 14, 2019
@Rajat-0
Copy link
Contributor Author

Rajat-0 commented Feb 14, 2019

@bskiba I have sperated it into two flags

@bskiba
Copy link
Member

bskiba commented Feb 18, 2019

From https://github.com/kubernetes/kubernetes/blob/92e0c231fa0fff744a1ce26955cf46c693dfa5e5/pkg/apis/admissionregistration/types.go#L239:6

Exactly one of url or service must be specified.

it seems that you can't register both by Service and URL. Does this work for you?

@Rajat-0
Copy link
Contributor Author

Rajat-0 commented Feb 18, 2019

Can we specify the url where it is currently installing the vpa-admission controller as the initial/default url?

@bskiba
Copy link
Member

bskiba commented Feb 18, 2019

Not sure I understand the question. The current way of registering the webhook is by specifying a Kubernetes Service to call. Registration by URL is an alternative, it shouldn't be used at the same time as Service is specified. That's why I am curious if this works for you in the current setup - seems it shouldn't work if you specify a not-empty URL.

We could introduce recognizing registration mode to avoid this clash - register as Service only if the url is not empty or have a --registerByURL flag

@Rajat-0
Copy link
Contributor Author

Rajat-0 commented Feb 19, 2019

Having a --registerByURL flag sounds good.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 25, 2019
@Rajat-0
Copy link
Contributor Author

Rajat-0 commented Feb 25, 2019

@bskiba I have updated the PR PTAL.

@Rajat-0 Rajat-0 force-pushed the admission branch 4 times, most recently from 03ee1ef to 916cd53 Compare February 25, 2019 11:19
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 25, 2019
@Rajat-0
Copy link
Contributor Author

Rajat-0 commented Feb 26, 2019

@bskiba PTAL

@bskiba
Copy link
Member

bskiba commented Feb 26, 2019

Probably last minor comments, please also resolve conflict.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 27, 2019
@Rajat-0
Copy link
Contributor Author

Rajat-0 commented Feb 27, 2019

@bskiba PTAL

@bskiba
Copy link
Member

bskiba commented Feb 27, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bskiba

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2019
@k8s-ci-robot k8s-ci-robot merged commit e535792 into kubernetes:master Feb 27, 2019
yaroslava-serdiuk pushed a commit to yaroslava-serdiuk/autoscaler that referenced this pull request Feb 22, 2024
@dependabot
Bump k8s.io/code-generator from 0.28.5 to 0.28.6 (kubernetes#1625)
ebe0d14 
Bumps [k8s.io/code-generator](https://github.com/kubernetes/code-generator) from 0.28.5 to 0.28.6.
- [Commits](kubernetes/code-generator@v0.28.5...v0.28.6)

---
updated-dependencies:
- dependency-name: k8s.io/code-generator
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bskiba bskiba bskiba approved these changes

@mwielgus mwielgus Awaiting requested review from mwielgus

Assignees

@bskiba bskiba

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Rajat-0 @bskiba @k8s-ci-robot