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.

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

cert-manager: upgrade to 1.5.4 #8069

Merged
merged 2 commits into from
Oct 12, 2021

Conversation

cristicalin
Copy link
Contributor

What type of PR is this?

/kind feature

What this PR does / why we need it:
This PR upgrades cert-manager to 1.5.4 and brings our templates closer to the upstream artifacts.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
This will need to be rebased after #8064 is merged since that one fixes a deployment issue of cert-manager.

Does this PR introduce a user-facing change?:

Cert-manager: upgrade to 1.5.4

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 9, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @cristicalin. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot requested review from EppO and oomichi October 9, 2021 21:22
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 9, 2021
@cristicalin
Copy link
Contributor Author

/cc @rtsp

@k8s-ci-robot
Copy link
Contributor

@cristicalin: GitHub didn't allow me to request PR reviews from the following users: rtsp.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @rtsp

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@rtsp
Copy link
Member

rtsp commented Oct 10, 2021

I really love your idea to simplify the imports using type: all. 😂😂

@cristicalin
Copy link
Contributor Author

I really love your idea to simplify the imports using type: all. 😂😂

I think the type field is something we never quite implemented so it's just a label, you can put whatever you want

@rtsp
Copy link
Member

rtsp commented Oct 10, 2021

Also, this is minor, but I think cert-manager should written in all lowercase.

Writing “cert-manager”

cert-manager should always be written in lowercase. Even when it would normally be capitalized such as in titles or at the start of sentences. A hyphen should always be used between the words, don’t replace it with a space or remove it.

https://cert-manager.io/v1.5-docs/faq/style/

@floryut
Copy link
Member

floryut commented Oct 11, 2021

I really love your idea to simplify the imports using type: all. 😂😂

I think the type field is something we never quite implemented so it's just a label, you can put whatever you want

you are quite right, I don't see any use in the python lib with this field..

Copy link
Member

@floryut floryut left a comment

Choose a reason for hiding this comment

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

/ok-to-test
Let's see if the CI is happy with this, but good job man! thx

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 11, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cristicalin, floryut

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 Oct 11, 2021
@cristicalin
Copy link
Contributor Author

@floryut I strongly suspect it will continue to fail until #8064 is merged and this PR is rebased on it.

@floryut
Copy link
Member

floryut commented Oct 11, 2021

@floryut I strongly suspect it will continue to fail until #8064 is merged and this PR is rebased on it.

Indeed, brain didn't start right away on monday, I remembered that this was a followup of the cert-manager pr fix after commenting 😄

@cristicalin cristicalin changed the title Cert-manager: upgrade to 1.5.4 cert-manager: upgrade to 1.5.4 Oct 11, 2021
@oomichi
Copy link
Contributor

oomichi commented Oct 11, 2021

#8064 is going to be merged.
Let's rebase this after that.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2021
@cristicalin
Copy link
Contributor Author

Done. Since, the previous MR did not address the docs cleanup I added a commit to this one to remove outdated information regarding the pre-created ClusterIssuer.

@floryut
Copy link
Member

floryut commented Oct 12, 2021

Done. Since, the previous MR did not address the docs cleanup I added a commit to this one to remove outdated information regarding the pre-created ClusterIssuer.

And some linting issue 😆

./docs/cert_manager.md:14 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]

@floryut floryut added the release-note-none Denotes a PR that doesn't merit a release note. label Oct 12, 2021
@cristicalin
Copy link
Contributor Author

And some linting issue 😆

Fixed now, ... mental note, add linter to the local pre-commit hook

@oomichi
Copy link
Contributor

oomichi commented Oct 12, 2021

Thanks for rebasing.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2021
@k8s-ci-robot k8s-ci-robot merged commit cee481f into kubernetes-sigs:master Oct 12, 2021
sakuraiyuta pushed a commit to sakuraiyuta/kubespray that referenced this pull request Apr 16, 2022
* cert-manager: update to 1.5.4

* cert-manager: remove outdated guidelines on creating an initial ClusterIssuer
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Jun 27, 2023
* cert-manager: update to 1.5.4

* cert-manager: remove outdated guidelines on creating an initial ClusterIssuer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants