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

⚠️(v3+ only) feat: update cert-manager crd to v1.0 in kubebuilder v3 #1667

Closed
wants to merge 1 commit into from

Conversation

MartinForReal
Copy link

Signed-off-by: MartinForReal fanshangxiang@gmail.cn
Fixed: #1666

Migrate cert-manager related CR to v1

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 8, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @MartinForReal!

It looks like this is your first PR to kubernetes-sigs/kubebuilder 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubebuilder has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @MartinForReal. 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 added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 8, 2020
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 8, 2020
@MartinForReal
Copy link
Author

/check-cla

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 8, 2020
@MartinForReal
Copy link
Author

/assign @droot

Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Really thank you for your contribution. 🥇 Just a few nits:

  • Note that The pkg/plugin/v2 ought to receive only fix bugs and cannot have breaking changes. All improvements and etc needs to be addressed only in pkg/plugin/v3. Could you please update it?
  • Also, note that it is not passing in the testdata. After change the templates we need to run make generate it will re-generate the testdata
  • Could you please add (v3 only) in the title as well?

PS.: the commits need to be squashed.

@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 Sep 9, 2020
@MartinForReal MartinForReal changed the title feat: update cert-manager crd to v1.0 feat: update cert-manager crd to v1.0 in kubebuilder v3 Sep 9, 2020
@MartinForReal
Copy link
Author

I've changed the commit accordingly. FYI @camilamacedo86 Thanks!

@camilamacedo86
Copy link
Member

camilamacedo86 commented Sep 9, 2020

Hi @MartinForReal,

It shows great. See that it is not passing in the check to verify if the testdata/ samples are updated. See: https://travis-ci.org/github/kubernetes-sigs/kubebuilder/jobs/725451668#L1003-L1038

To update these samples we run make generate. Any change on the templates requires we run this command to do the pr. Could you please check it? Also, could you please add on the title (v3+ only)? And we need to start to use :


All code PR must be labeled with 🐛 (patch fixes), ✨ (backwards-compatible features), or ⚠️ (breaking changes)
For a quick PR template suggesting the right information, use one of these PR templates:
Breaking Changes/Features
Backwards-Compatible Features
Bug fixes
Documentation Changes
Test/Build/Other Changes

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 9, 2020
@MartinForReal MartinForReal reopened this Sep 9, 2020
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 9, 2020
@MartinForReal MartinForReal changed the title feat: update cert-manager crd to v1.0 in kubebuilder v3 ⚠️(v3+ only) feat: update cert-manager crd to v1.0 in kubebuilder v3 Sep 9, 2020
@MartinForReal
Copy link
Author

/assign @camilamacedo86

@MartinForReal
Copy link
Author

Hi @camilamacedo86,

Thanks for your instructions~

@camilamacedo86
Copy link
Member

/ok-to-test

@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 Sep 9, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MartinForReal
To complete the pull request process, please assign camilamacedo86
You can assign the PR to them by writing /assign @camilamacedo86 in a comment when ready.

The full list of commands accepted by this bot can be found 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

@camilamacedo86
Copy link
Member

camilamacedo86 commented Sep 9, 2020

It shows ok now. Great work. Also, I checked in https://cert-manager.io/docs/installation/upgrading/. Could you please add this link in the first comment as well just for we keep easy the info if someone needs to check that after?

Also, note that it is not passing in the e2e tests which means that is not working well https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kubebuilder/1667/pull-kubebuilder-e2e-k8s-1-14-1/1303636850719592448#1:build-log.txt%3A1017. See that the v1 version required CRD v1 as well.

cert-manager now uses apiextensions.k8s.io/v1 to install CRDs inside Kubernetes

So, it is blocked by #1644.

c/c @estroz

Signed-off-by: MartinForReal <fanshangxiang@gmail.com>
@k8s-ci-robot
Copy link
Contributor

@MartinForReal: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubebuilder-e2e-k8s-1-16-2 98dd786 link /test pull-kubebuilder-e2e-k8s-1-16-2
pull-kubebuilder-e2e-k8s-1-15-3 98dd786 link /test pull-kubebuilder-e2e-k8s-1-15-3
pull-kubebuilder-e2e-k8s-1-14-1 98dd786 link /test pull-kubebuilder-e2e-k8s-1-14-1
pull-kubebuilder-e2e-k8s-1-17-0 98dd786 link /test pull-kubebuilder-e2e-k8s-1-17-0

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@k8s-ci-robot
Copy link
Contributor

@MartinForReal: PR needs rebase.

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 added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2020
Comment on lines +34 to +36
const certmanagerVersion = "v1.0.1"
const certmanagerURL = "https://github.com/jetstack/cert-manager/releases/download/%s/cert-manager-legacy.yaml"

Copy link
Member

@camilamacedo86 camilamacedo86 Oct 17, 2020

Choose a reason for hiding this comment

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

it should not be added here. See;

const certmanagerVersion = "v0.11.0"

We need to update it in the utils. I'd recommend that the func InstallCertManager() would check the project config and would apply the cert-manager accordingly. if project.isV2 then, apply the current version used, otherwise apply the upper version.

Copy link
Member

Choose a reason for hiding this comment

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

@@ -31,6 +31,9 @@ import (
"sigs.k8s.io/kubebuilder/test/e2e/utils"
)

const certmanagerVersion = "v1.0.1"
const certmanagerURL = "https://github.com/jetstack/cert-manager/releases/download/%s/cert-manager-legacy.yaml"
Copy link
Contributor

Choose a reason for hiding this comment

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

@MartinForReal you'll be able to customize which set of manifests to use (current vs legacy) with kubectl version once #1644 is merged.

const prometheusOperatorVersion = "0.33"
const certmanagerURL = "https://github.com/jetstack/cert-manager/releases/download/%s/cert-manager.yaml"
const prometheusOperatorURL = "https://raw.githubusercontent.com/coreos/prometheus-operator/release-%s/bundle.yaml"
Copy link
Member

Choose a reason for hiding this comment

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

We need to keep this one and then, in the e2e tests for the v3-alpha we should use the upper version and for the v2+ we should keep using this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cert-manager related configuration should be migrated to cert-manager.io/v1
5 participants