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

OCPVE-220: remove unused certificates #413

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

suleymanakbas91
Copy link
Contributor

@suleymanakbas91 suleymanakbas91 commented Sep 8, 2023

This PR removes the initcontainer self-signed-cert-generator from TopoLVM Controller. This is because the certs are used for TopoLVM webhooks which are not deployed in LVMS.

Also, this PR removes the unused webhook and certmanager files to simplify the codebase.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 8, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 8, 2023

@suleymanakbas91: This pull request references OCPVE-220 which is a valid jira issue.

In response to this:

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.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 8, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 8, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 8, 2023
@suleymanakbas91 suleymanakbas91 force-pushed the remove-certs branch 4 times, most recently from 867c3df to 3288d69 Compare September 11, 2023 10:43
@suleymanakbas91 suleymanakbas91 marked this pull request as ready for review September 11, 2023 10:44
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 11, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 11, 2023

@suleymanakbas91: This pull request references OCPVE-220 which is a valid jira issue.

In response to this:

This PR removes the initcontainer self-signed-cert-generator from TopoLVM Controller. This is because the certs are used for TopoLVM webhooks which are not deployed in LVMS. As there is no option to disable this from TopoLVM, we just pass the existing webhook certs from LVMS to make the controller start, but the certs are not used afterwards.

Also, this PR removes the unused webhook and certmanager files to simplify the codebase.

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.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 11, 2023

@suleymanakbas91: This pull request references OCPVE-220 which is a valid jira issue.

In response to this:

This PR removes the initcontainer self-signed-cert-generator from TopoLVM Controller. This is because the certs are used for TopoLVM webhooks which are not deployed in LVMS. As there is no option to disable this from TopoLVM, we just pass the existing webhook certs from LVMS to make the controller start, but the certs are not used afterward.

Also, this PR removes the unused webhook and certmanager files to simplify the codebase.

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.

Copy link
Contributor

@jakobmoellerdev jakobmoellerdev left a comment

Choose a reason for hiding this comment

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

Less certs = more good!

Had a question regarding the necessary mount though since Im pretty sure this can be fixed in upstream. They already have flags in their helm chart for disabling it but maybe we also should contribute a code path that allows also omitting the mount. WDYT?

controllers/topolvm_controller.go Outdated Show resolved Hide resolved
docs/design/lvm-operator-manager.md Show resolved Hide resolved
@suleymanakbas91
Copy link
Contributor Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 11, 2023
@suleymanakbas91 suleymanakbas91 force-pushed the remove-certs branch 2 times, most recently from a273eb1 to 47feb92 Compare September 19, 2023 12:14
@suleymanakbas91
Copy link
Contributor Author

/test all

@suleymanakbas91
Copy link
Contributor Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 22, 2023

@suleymanakbas91: This pull request references OCPVE-220 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

This PR removes the initcontainer self-signed-cert-generator from TopoLVM Controller. This is because the certs are used for TopoLVM webhooks which are not deployed in LVMS.

Also, this PR removes the unused webhook and certmanager files to simplify the codebase.

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.

@suleymanakbas91
Copy link
Contributor Author

/retest

@suleymanakbas91
Copy link
Contributor Author

/retest

1 similar comment
@suleymanakbas91
Copy link
Contributor Author

/retest

@codecov-commenter
Copy link

Codecov Report

Merging #413 (8c621ab) into main (9677ed3) will decrease coverage by 0.38%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #413      +/-   ##
==========================================
- Coverage   63.68%   63.31%   -0.38%     
==========================================
  Files          28       28              
  Lines        2360     2336      -24     
==========================================
- Hits         1503     1479      -24     
  Misses        709      709              
  Partials      148      148              
Files Changed Coverage Δ
controllers/topolvm_controller.go 97.20% <100.00%> (-0.29%) ⬇️
controllers/vgmanager_daemonset.go 100.00% <100.00%> (ø)

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 22, 2023

@suleymanakbas91: all tests passed!

Full PR test history. Your PR dashboard.

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 22, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jakobmoellerdev, suleymanakbas91

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

@jakobmoellerdev
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2023
@openshift-merge-robot openshift-merge-robot merged commit 3448dbf into openshift:main Sep 22, 2023
7 checks passed
@suleymanakbas91 suleymanakbas91 deleted the remove-certs branch September 22, 2023 14:36
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. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm 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
Development

Successfully merging this pull request may close these issues.

5 participants