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

⚠️ (go/v3-alpha) bump cert-manager CRs to v1 #1840

Merged
merged 1 commit into from
Nov 18, 2020
Merged

⚠️ (go/v3-alpha) bump cert-manager CRs to v1 #1840

merged 1 commit into from
Nov 18, 2020

Conversation

estroz
Copy link
Contributor

@estroz estroz commented Nov 18, 2020

This PR bumps cert-manager CR apiVersion to v1 from v1beta2 for go/v3-alpha. TestContext has been updated to be cluster version-aware such that the correct cert-manager bundle URL is applied.

  • pkg/plugin/v3/scaffolds/internal/templates/config: cert-manager CRs have apiVersion v1
  • test/e2e/utils: KubectlVersion -> KubernetesVersion, which now gets created in NewTestContext. cert-manager install funcs use this version to get the correct bundle for a particular cluster version
  • test/e2e/{v2,v3}: update {Install,Uninstall}CertManager() to include new 'hasv1beta2CRs' boolean arg, which depends on plugin being tested

Closes #1666
Closes #1667

@k8s-ci-robot k8s-ci-robot added 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 18, 2020
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2020
@estroz
This commit bumps cert-manager CR apiVersion to v1 from v1beta2
ea3a94a 
for go/v3-alpha. TestContext has been updated to be cluster version-
aware such that the correct cert-manager bundle URL is applied.

pkg/plugin/v3/scaffolds/internal/templates/config: cert-manager
CRs have apiVersion v1

test/e2e/utils: KubectlVersion -> KubernetesVersion, which now
gets created in NewTestContext. cert-manager install funcs use this
version to get the correct bundle for a particular cluster version

test/e2e/{v2,v3}: update {Install,Uninstall}CertManager() to include
new 'hasv1beta2CRs' boolean arg, which depends on plugin being tested
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2020
@estroz
Copy link
Contributor Author

estroz commented Nov 18, 2020

Didn't see any recent activity on #1667 so I rebased and updated that code.

/ping @MartinForReal

camilamacedo86
camilamacedo86 reviewed Nov 18, 2020
View reviewed changes
test/e2e/utils/test_context.go

const (
prometheusOperatorVersion = "0.33"
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.

wdyt about we keep all const on the top of this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Usually I would do that, but these variables are only relevant to two methods in the entire file. Ditto for cert-manager variables.

Copy link
Member

Choose a reason for hiding this comment

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

IMO the const centralized on top is a good practice that should be always followed.
But lets no block it because of this nit.

camilamacedo86
camilamacedo86 approved these changes Nov 18, 2020
View reviewed changes
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.

IMO we just need to address a nit: https://github.com/kubernetes-sigs/kubebuilder/pull/1840/files#r525623927

Otherwise, it shows great
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, estroz

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:
  • OWNERS [camilamacedo86,estroz]

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 18, 2020
@k8s-ci-robot k8s-ci-robot merged commit 5f26dfc into kubernetes-sigs:master Nov 18, 2020
This was referenced Dec 4, 2020
Merged
Closed
Closed
@fabriziopandini fabriziopandini mentioned this pull request Dec 15, 2020
Merged
@camilamacedo86 camilamacedo86 mentioned this pull request Feb 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@joelanford joelanford Awaiting requested review from joelanford

Assignees

@camilamacedo86 camilamacedo86

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cert-manager related configuration should be migrated to cert-manager.io/v1
3 participants
@estroz @k8s-ci-robot @camilamacedo86