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.

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

Allow partition id as a parameter for publish command. #2515

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yaruno
Copy link
Contributor

@yaruno yaruno commented Apr 23, 2024

Should not allow both partition key and partition id at the same time.

Summary

Added partition id as possible parameter for publish command.

Changes

We'll check if partition key and id are given as parameters and give suitable error msg. It's one or the other but not both as parameter options.

Limitations and future improvements

Provide a bullet list or description of known omissions or limitations of the
solution and/or any ideas for future improvement. Leave section ouf if
not applicable.

Checklist before requesting a review

  • Is this a breaking change? If it is, be clear in summary.
  • Read through code myself one more time.
  • Make sure any and all TODO comments left behind are meant to be left in.
  • Has reasonable passing test coverage?
  • Updated changelog if applicable.
  • Updated documentation if applicable.

…ow both partition key and partition id at the same time.
Copy link

linear bot commented Apr 23, 2024

@github-actions github-actions bot added the cli-tools Related to CLI Tools Package label Apr 23, 2024
@harbu harbu self-requested a review June 25, 2024 11:13
Copy link
Contributor

@harbu harbu left a comment

Choose a reason for hiding this comment

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

still want to get this merged?

check eslint; see comment about createFnParseInt

@@ -63,4 +73,5 @@ createClientCommand(async (client: StreamrClient, streamId: string, options: Opt
.arguments('<streamId>')
.description('publish to a stream by reading JSON messages from stdin line-by-line')
.option('-k, --partition-key-field <string>', 'field name in each message to use for assigning the message to a stream partition')
.option('-p, --partition [partition]', 'partition')
Copy link
Contributor

Choose a reason for hiding this comment

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

Move the use of createFnParseInthere. See streamr-stream-subscribe.ts for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli-tools Related to CLI Tools Package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants