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

Allow passing through a message key for Kafka executor #1021

Merged
merged 5 commits into from
Nov 3, 2021
Merged

Allow passing through a message key for Kafka executor #1021

merged 5 commits into from
Nov 3, 2021

Conversation

ConductorPete
Copy link
Contributor

@ConductorPete ConductorPete commented Oct 5, 2021 •
edited
Loading

Enable the ability to use a message key in the Kafka executor. The key is fairly useful/core functionality for Kafka:

yvanoers
yvanoers requested changes Oct 9, 2021
View reviewed changes
website/content/usage/executors/kafka.md Outdated Show resolved Hide resolved
builtin/bins/dkron-executor-kafka/kafka.go Show resolved Hide resolved
@ConductorPete ConductorPete requested a review from yvanoers October 9, 2021 19:49
yvanoers
yvanoers previously approved these changes Oct 10, 2021
View reviewed changes
Copy link
Collaborator

@yvanoers yvanoers left a comment

Choose a reason for hiding this comment

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

LGTM!

@ConductorPete
Copy link
Contributor Author

ConductorPete commented Oct 10, 2021 •
edited
Loading

Thanks @yvanoers! Looks like the (unrelated) test TestRootCA is currently broken due to the cert at https://untrusted-root.badssl.com/ having expired as of 2021-10-08T23:08:50Z and not yet renewed. I'll assume that this will be fixed soon, and will retry the build later - unless you have another suggestion?

See chromium/badssl.com#477 for expired cert issue.

vcastellm
vcastellm previously approved these changes Oct 11, 2021
View reviewed changes
Copy link
Member

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

LGTM

@vcastellm
Copy link
Member

@ConductorPete thanks for the PR!

Regarding the SSL, yes, update needed

@ConductorPete ConductorPete dismissed stale reviews from vcastellm and yvanoers via 479b846 October 12, 2021 23:34
@ConductorPete
Copy link
Contributor Author

Cert issue has been fixed

vcastellm
vcastellm approved these changes Oct 13, 2021
View reviewed changes
Copy link
Member

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

LGTM

@ConductorPete
Copy link
Contributor Author

@Victorcoder @yvanoers I'm not sure about your process/milestones/etc, but is there anything else required from my end to get this merged in?

@yvanoers yvanoers merged commit 5703402 into distribworks:master Nov 3, 2021
@yvanoers
Copy link
Collaborator

yvanoers commented Nov 3, 2021

@ConductorPete I typically refrain from merging. In this case however, @Victorcoder has already seen and approved the PR, so I'm ok with doing it.
I don't know when this will end up in a release, though.

@ConductorPete
Copy link
Contributor Author

Thanks for the merge!

@vcastellm
Copy link
Member

Yeah thx @yvanoers, been a bit busy lately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yvanoers yvanoers yvanoers approved these changes

@vcastellm vcastellm vcastellm approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@ConductorPete @vcastellm @yvanoers