Skip to content

✨ Add cert-manager as a dependency to helm-chart#144

Merged
k8s-ci-robot merged 2 commits into
kubernetes-sigs:mainfrom
Fedosin:helm_chart
Jul 12, 2023
Merged

✨ Add cert-manager as a dependency to helm-chart#144
k8s-ci-robot merged 2 commits into
kubernetes-sigs:mainfrom
Fedosin:helm_chart

Conversation

@Fedosin

@Fedosin Fedosin commented Jun 7, 2023

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

To work properly CAPI Operator needs cert-manager. To ensure it is installed, we add a cert-manager dependency to the helm chart.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Partially-Fixes: #130

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 7, 2023
@k8s-ci-robot k8s-ci-robot requested review from damdo and neolit123 June 7, 2023 18:05
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 7, 2023
@Fedosin

Fedosin commented Jun 7, 2023

Copy link
Copy Markdown
Contributor Author

Blocked by #145

@damdo

damdo commented Jun 7, 2023

Copy link
Copy Markdown
Member

How does this behave when the cluster already has cert manager installed in a different way and not through Helm?
How does this behave when the cluster already has cert manager installed via Helm but that's a different version from the one linked here as a dependency?

@Fedosin

Fedosin commented Jun 8, 2023

Copy link
Copy Markdown
Contributor Author

Yeah, I think this change just over-complicates the helm chart. Moreover, they don't support installing CRDs and some manual actions are required anyway.
I'm going to close this PR.

@Fedosin Fedosin closed this Jun 8, 2023
@Fedosin Fedosin reopened this Jun 23, 2023
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 23, 2023
@Fedosin Fedosin force-pushed the helm_chart branch 2 times, most recently from 53c95df to 25f4e93 Compare June 24, 2023 00:26
@Fedosin

Fedosin commented Jun 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Seems working now. I tried it with helm install cluster-api-operator out/package/cluster-api-operator-0.3.0.tgz --set cert-manager.enabled=true --create-namespace -n capi-operator-system --set cert-manager.namespace=cert-manager
Everything is installed successfully in related namespaces.

@alexander-demicev

alexander-demicev commented Jun 26, 2023

Copy link
Copy Markdown
Contributor

@Fedosin is there an option to disable cert-manager installation?

@g-gaston

g-gaston commented Jun 26, 2023

Copy link
Copy Markdown

/lgtm
thanks a lot for doing this!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2023
@k8s-ci-robot

k8s-ci-robot commented Jun 26, 2023

Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: 23c00648359312ea9ea42e206879085d3e237eed

@Fedosin

Fedosin commented Jun 26, 2023

Copy link
Copy Markdown
Contributor Author

@alexander-demicev Helm installation is disabled by default: https://github.com/kubernetes-sigs/cluster-api-operator/pull/144/files#diff-eb3428ea28fccc0ad2deffddc87823203d05dc4a3b5e9d73f3f093ecc40f5a53R3

You need to explicitly set cert-manager.enabled=true to install it. I provided an example of helm command that does it: #144 (comment)

@alexander-demicev

alexander-demicev commented Jul 4, 2023

Copy link
Copy Markdown
Contributor

I'm okay with merging this if others have no objections @damdo @furkatgofurov7 we should also update docs in a follow-up PR

@alexander-demicev

alexander-demicev commented Jul 7, 2023

Copy link
Copy Markdown
Contributor

We can also download cert-manager chart during make release instead of keeping it in repo

Fedosin added 2 commits July 9, 2023 23:05
@Fedosin
chore: add a script to manage cert-manager dependency in the helm chart
c50533a
@Fedosin
feat: add cert-manager as a dependency to helm-chart
e8d8e01 
To work properly CAPI Operator needs cert-manager. To ensure it is
installed, we add a cert-manager dependency to the helm chart.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 9, 2023
@k8s-ci-robot k8s-ci-robot requested a review from g-gaston July 9, 2023 21:07
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 9, 2023
@Fedosin

Fedosin commented Jul 9, 2023

Copy link
Copy Markdown
Contributor Author

@alexander-demicev yeah, it makes sense. I also did it for cert-manager-v1.12.2.tgz binary, so we won't store it in our repo.

alexander-demicev
alexander-demicev approved these changes Jul 12, 2023
View reviewed changes

@alexander-demicev alexander-demicev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot

k8s-ci-robot commented Jul 12, 2023

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexander-demicev

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 Jul 12, 2023
furkatgofurov7
furkatgofurov7 reviewed Jul 12, 2023
View reviewed changes

@furkatgofurov7 furkatgofurov7 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 12, 2023
@k8s-ci-robot

k8s-ci-robot commented Jul 12, 2023

Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: 8ed95d9e6f9a901c6b2984fced81c953dade444e

@k8s-ci-robot k8s-ci-robot merged commit 0127276 into kubernetes-sigs:main Jul 12, 2023
@g-gaston g-gaston mentioned this pull request Jul 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@furkatgofurov7 furkatgofurov7 furkatgofurov7 left review comments

@alexander-demicev alexander-demicev alexander-demicev approved these changes

@damdo damdo Awaiting requested review from damdo

@neolit123 neolit123 Awaiting requested review from neolit123

@g-gaston g-gaston Awaiting requested review from g-gaston

Assignees

@g-gaston g-gaston

@furkatgofurov7 furkatgofurov7

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.

Helm chart support more comprehensive installation

6 participants

@Fedosin @damdo @alexander-demicev @g-gaston @k8s-ci-robot @furkatgofurov7