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

Upgrade kube-webhook-certgen #234

Merged
merged 1 commit into from
Jan 11, 2022
Merged

Upgrade kube-webhook-certgen #234

merged 1 commit into from
Jan 11, 2022

Conversation

chaunceyjiang
Copy link
Contributor

Fix #233

Fix https://kubesphere.com.cn/forum/d/6449-porter

Apparently the upstream is no longer maintained, switching to ingress-nginx fork jet/kube-webhook-certgen#30

Test

image

I have pushed the container image "k8s.gcr.io/ingress-nginx/kube-webhook-certgen:v1.1.1" to "kubespheredev/kube-webhook-certgen:v1.1.1"

image

LinuxSuRen
LinuxSuRen reviewed Jan 5, 2022
View reviewed changes
deploy/porter.yaml Outdated Show resolved Hide resolved
liangyuanpeng
liangyuanpeng reviewed Jan 5, 2022
View reviewed changes
Copy link
Member

@liangyuanpeng liangyuanpeng left a comment

Choose a reason for hiding this comment

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

Thanks for your work and add some comment,

deploy/porter.yaml Outdated Show resolved Hide resolved
@liangyuanpeng
Copy link
Member

https://github.com/openelb/openelb/pull/234/checks?check_run_id=4710766538

Need to sign for DCO.

@liangyuanpeng liangyuanpeng mentioned this pull request Jan 6, 2022
Closed
Upgrade kube-webhook-certgen
2b7e6f3 
Signed-off-by: chaunceyjiang <[email protected]>
@chaunceyjiang
Copy link
Contributor Author

https://github.com/openelb/openelb/pull/234/checks?check_run_id=4710766538

Need to sign for DCO.

ok

@FeynmanZhou
Copy link
Member

/LGTM
Hi @zheng1 , could you please help to review it?

@zheng1
Copy link
Collaborator

zheng1 commented Jan 6, 2022

/approve

Thank you for your contribution

@FeynmanZhou
Copy link
Member

@LinuxSuRen Could you please help to add your approval here? Thanks.

@LinuxSuRen
Copy link
Contributor

@LinuxSuRen Could you please help to add your approval here? Thanks.

I don't block this PR. And I have no commnit permission on this repo.

But I am still feeling confused about the version part.

jettech/kube-webhook-certgen:v1.5.0 is higher than k8s.gcr.io/ingress-nginx/kube-webhook-certgen:v1.1.1 from the image tag perspective. And I didn't see any quick way to confirm v1.1.1 is higher than v1.5.0

@LinuxSuRen
Copy link
Contributor

By the way, it looks like there is no bot response here. Perhaps the following components have some issues:

  • webhook
  • no configuration on the prow

@chaunceyjiang
Copy link
Contributor Author

@LinuxSuRen Could you please help to add your approval here? Thanks.

I don't block this PR. And I have no commnit permission on this repo.

But I am still feeling confused about the version part.

jettech/kube-webhook-certgen:v1.5.0 is higher than k8s.gcr.io/ingress-nginx/kube-webhook-certgen:v1.1.1 from the image tag perspective. And I didn't see any quick way to confirm v1.1.1 is higher than v1.5.0

for example
https://github.com/kubernetes/ingress-nginx/blob/main/charts/ingress-nginx/values.yaml#L604-L608

@LinuxSuRen
Copy link
Contributor

for example
https://github.com/kubernetes/ingress-nginx/blob/main/charts/ingress-nginx/values.yaml#L604-L608

I'm not familiar with kube-webhook-certgen and this repo. I guess I'm not a perfect reviewer. But I still don't understand the version changing.

@chaunceyjiang
Copy link
Contributor Author

for example
https://github.com/kubernetes/ingress-nginx/blob/main/charts/ingress-nginx/values.yaml#L604-L608

I'm not familiar with kube-webhook-certgen and this repo. I guess I'm not a perfect reviewer. But I still don't understand the version changing.

You can review this pr kubernetes/ingress-nginx#7475 and https://github.com/kubernetes/ingress-nginx/blob/main/images/kube-webhook-certgen/Makefile#L22.
So the version of k8s.gcr.io/ingress-nginx/kube-webhook-certgen has nothing to do with the version of jettech/kube-webhook-certgen

@zheng1
Copy link
Collaborator

zheng1 commented Jan 10, 2022

/lgtm
/approve

@ks-ci-bot ks-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2022
@ks-ci-bot
Copy link
Collaborator

LGTM label has been added.

Git tree hash: 3e9e353f9926910ee5f216384033708199915049

@ks-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chaunceyjiang, FeynmanZhou, liangyuanpeng, zheng1

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

@ks-ci-bot ks-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2022
@liangyuanpeng
Copy link
Member

liangyuanpeng commented Jan 11, 2022 •
edited
Loading

I'm not familiar with kube-webhook-certgen and this repo. I guess I'm not a perfect reviewer. But I still don't understand the version changing.

@LinuxSuRen
k8s.gcr.io/ingress-nginx/kube-webhook-certgen:v1.1.1 is the new version and it base on jettech/kube-webhook-certgen:v1.5.2(the latest version today).

Check this PR Migrate the webhook-certgen program to inside ingress repo and compare the timeline for repo of jet/kube-webhook-certgen

@zheng1
Copy link
Collaborator

zheng1 commented Jan 11, 2022

It looks like the ci-bot is having troubles, I'll merge manually first

@zheng1 zheng1 merged commit 877b5ee into openelb:master Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LinuxSuRen LinuxSuRen LinuxSuRen left review comments

@FeynmanZhou FeynmanZhou FeynmanZhou approved these changes

@liangyuanpeng liangyuanpeng liangyuanpeng approved these changes

Assignees

@zheng1 zheng1

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

install porter err porter-admission-patch
6 participants
@chaunceyjiang @liangyuanpeng @FeynmanZhou @zheng1 @LinuxSuRen @ks-ci-bot