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

add principal and keytab to kafka output #1059

Merged
merged 7 commits into from
Jul 29, 2022
Merged

add principal and keytab to kafka output #1059

merged 7 commits into from
Jul 29, 2022

Conversation

bryce-wilkinson-443
Copy link
Contributor

@bryce-wilkinson-443 bryce-wilkinson-443 commented Jul 22, 2022 •
edited
Loading

Q A
Bug fix? no
New feature? yes
API breaks? no
Deprecations? no
Related tickets fixes ##1058
License Apache 2.0

What's in this PR?

Add principal and Keytab fields to Output and Cluster Output CRD's

Why?

These fields are necessary if the kafka cluster is using kerberos

Checklist

  • Code meets the Developer Guide
  • User guide and development docs updated (if needed)
  • Related Helm chart(s) updated (if needed)

To Do

  • Test

@CLAassistant
Copy link

CLAassistant commented Jul 22, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

siliconbrain
siliconbrain previously approved these changes Jul 22, 2022
View reviewed changes
Copy link
Contributor

@siliconbrain siliconbrain left a comment

Choose a reason for hiding this comment

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

lgtm

@siliconbrain
Copy link
Contributor

@bryce-wilkinson-443 Please run make generate to regenerate the VFS embedding the manifests and push the changes. That should fix the build.

@aslafy-z aslafy-z changed the title add principal and keytab add principal and keytab to kafka output Jul 22, 2022
@ahma ahma self-requested a review July 22, 2022 21:00
ahma
ahma previously approved these changes Jul 22, 2022
View reviewed changes
Copy link
Contributor

@ahma ahma left a comment

Choose a reason for hiding this comment

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

Thanks @bryce-wilkinson-443 lgtm

@ahma
Copy link
Contributor

ahma commented Jul 22, 2022

@bryce-wilkinson-443 could you please run the make generate ?

@bryce-wilkinson-443 bryce-wilkinson-443 dismissed stale reviews from ahma and siliconbrain via 56dfff8 July 25, 2022 00:42
@bryce-wilkinson-443
Copy link
Contributor Author

bryce-wilkinson-443 commented Jul 25, 2022 •
edited
Loading

Thanks @ahma @siliconbrain I have ran make generate and pushed however it still seems to be failing at the diff stage

@siliconbrain
Copy link
Contributor

@bryce-wilkinson-443 Because you've merged from master and didn't run make generate again 😄 Now, you should run

make generate
make manifests
make generate

I know, it's a little convoluted, but that's how it is 🤷

@siliconbrain
Copy link
Contributor

@bryce-wilkinson-443 you're still missing a go fmt 🙂

@bryce-wilkinson-443
Copy link
Contributor Author

Thank you

siliconbrain
siliconbrain previously approved these changes Jul 25, 2022
View reviewed changes
@siliconbrain
Copy link
Contributor

siliconbrain commented Jul 25, 2022 •
edited
Loading

@bryce-wilkinson-443 Now you just have to sign the CLA and finish the checklist in the PR description

@ahma ahma self-requested a review July 27, 2022 19:28
ahma
ahma previously approved these changes Jul 27, 2022
View reviewed changes
@bryce-wilkinson-443
make++
2728fd7 
make generate

make manifests

make generate

Update kafka.go
@bryce-wilkinson-443 bryce-wilkinson-443 dismissed stale reviews from ahma and siliconbrain via 2728fd7 July 28, 2022 03:48
@bryce-wilkinson-443
Copy link
Contributor Author

bryce-wilkinson-443 commented Jul 28, 2022 •
edited
Loading

Thanks for all your help, I have confirmed it's working. I tried to squash a few commits to clean it up. But unable to for some reason.

PR is now ready to merge

@siliconbrain
Copy link
Contributor

@bryce-wilkinson-443 we're just waiting for you to sign the CLA

@bryce-wilkinson-443
Copy link
Contributor Author

Thanks, CLA has been signed

@ahma ahma self-requested a review July 29, 2022 08:41
@ahma ahma merged commit 14f8ed0 into kube-logging:master Jul 29, 2022
@aslafy-z aslafy-z mentioned this pull request Apr 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahma ahma ahma approved these changes

@siliconbrain siliconbrain siliconbrain left review comments

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

Successfully merging this pull request may close these issues.
4 participants
@bryce-wilkinson-443 @CLAassistant @siliconbrain @ahma