-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add option to set kinsumer client name #383
Conversation
}) | ||
} | ||
} | ||
|
||
func TestClientNamePassThrough(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of this additional test? What does it provide that isn't included in the other test?
Can its purpose be documented in a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed on call - the test above does all we need, we can remove this one.
t.Fatalf("failed to CreateComponent with error: %q", err.Error()) | ||
} | ||
|
||
// workaround since clientName is unexported by Kinsumer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not asking you to implement anything, but have a think about other possible options to verify the feature, and when we catch up let's think about what the simplest options might be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Followup - we discussed different ways of thinking on the call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
1653f96
to
2baf306
Compare
2baf306
to
212b3df
Compare
Jira ref: PDP-1581