-
Notifications
You must be signed in to change notification settings - Fork 222
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
Upgrade KCL Java dependency from 2.4.4 to 2.5.1 and Added streamArn support in KCL Python #221
Conversation
@@ -135,6 +135,11 @@ | |||
<artifactId>apache-client</artifactId> | |||
<version>${awssdk.version}</version> | |||
</dependency> | |||
<dependency> | |||
<groupId>software.amazon.awssdk</groupId> | |||
<artifactId>arns</artifactId> |
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.
I assume that this is where the new functionality comes in to read in the arn. I also assume that it is plural because it can be a list. Is there anywhere where we document clearly that this is the case
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.
-
Correct, this dependency is added for streamArn support.
-
It is not plural because it can be a list. That is what the artifact is called (and was named by whomever is the creator of software.amazon.awssdk). It refers to the arns package: https://central.sonatype.com/artifact/software.amazon.awssdk/arns
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.
Is there any way we can document this use case. I am confused on how this will surface well in the code to be documented for future developers and users. For example can we comment that link into the pom file?
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.
Maven's central repository is the link i provided above. the Pom.xml pulls dependencies from there. Therefore it's already implied that the link above refers to the dependency in the XML. an explicit link should not be needed.
Description of changes:
Testing
Scenario #0 - Existing Functionality - streamName set and streamArn propert DNE
Config:
Output
Scenario #1 - streamName and streamArn passed in. KCL uses streamArn
Config:
Output
Scenario #2 - Only streamArn is passed in
Config
Ouput
Scenario #3 - Only streamName is passed in
Config
Output
Scenario #4 - Neither are set
Config:
Output:
==========
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.