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

kafkauser: copy ca to secret #1028

Merged
merged 11 commits into from
Aug 18, 2023
Merged

kafkauser: copy ca to secret #1028

merged 11 commits into from
Aug 18, 2023

Conversation

LuciferInLove
Copy link
Contributor

Description

  • Copy istio CA from the cert-manager ClusterIssuer Secret to KafkaUser Secret
  • Use cert-manager-namespace flag in both certmanagerpki and k8scsrpki packages

Type of Change

  • New Feature

Checklist

  • I have read the contributing guidelines
  • Existing issues have been referenced (where applicable)
  • I have verified this change is not present in other open pull requests
  • Functionality is documented
  • All code style checks pass
  • All new and existing tests pass

@LuciferInLove LuciferInLove requested a review from a team as a code owner July 31, 2023 15:42
Kuvesz
Kuvesz previously approved these changes Aug 1, 2023
View reviewed changes
Copy link
Contributor

@Kuvesz Kuvesz left a comment

Choose a reason for hiding this comment

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

LGTM, left a comment.

pkg/pki/k8scsrpki/k8scsr_user.go Outdated Show resolved Hide resolved
Kuvesz
Kuvesz previously approved these changes Aug 1, 2023
View reviewed changes
panyuenlau
panyuenlau previously approved these changes Aug 3, 2023
View reviewed changes
Copy link
Member

@panyuenlau panyuenlau left a comment

Choose a reason for hiding this comment

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

Left one question, but overall lgtm

pkg/pki/k8scsrpki/k8scsr_user.go Outdated Show resolved Hide resolved
@LuciferInLove LuciferInLove dismissed stale reviews from panyuenlau and Kuvesz via 30441ce August 4, 2023 11:00
panyuenlau
panyuenlau previously approved these changes Aug 4, 2023
View reviewed changes
pregnor
pregnor previously requested changes Aug 7, 2023
View reviewed changes
Copy link
Member

@pregnor pregnor left a comment

Choose a reason for hiding this comment

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

I'm not a cert expert, @bartam1 is better in this stuff, found 1 thing, otherwise LGTM to me

pkg/pki/k8scsrpki/k8scsr_user.go Show resolved Hide resolved
@pregnor pregnor dismissed their stale review August 14, 2023 16:14

discussed in comments

pregnor
pregnor previously approved these changes Aug 14, 2023
View reviewed changes
@LuciferInLove LuciferInLove dismissed stale reviews from pregnor and panyuenlau via eaf507b August 15, 2023 08:31
bartam1
bartam1 approved these changes Aug 16, 2023
View reviewed changes
Copy link
Contributor

@bartam1 bartam1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@pregnor
Copy link
Member

pregnor commented Aug 17, 2023

IMO it's mergable but I didn't dare to merge because didn't want to cause any headaches to #1020

@LuciferInLove
Copy link
Contributor Author

IMO it's mergable but I didn't dare to merge because didn't want to cause any headaches to #1020

Such a long-suffering request... 😃
If we don't plan to move to cert-manager anymore, I can close this PR. Otherwise, @bartam1 can approve if it creates more mess for #1020 or not. As another option, I can merge it later.

@bartam1 bartam1 merged commit 17bcda4 into master Aug 18, 2023
4 checks passed
@bartam1 bartam1 deleted the kafkauser-copy-ca-to-secret branch August 18, 2023 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pregnor pregnor pregnor approved these changes

@bartam1 bartam1 bartam1 approved these changes

@Kuvesz Kuvesz Awaiting requested review from Kuvesz

@panyuenlau panyuenlau Awaiting requested review from panyuenlau

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@LuciferInLove @pregnor @panyuenlau @Kuvesz @bartam1