Skip to content

fix(kafka topic create): erroneous flag validation#1258

Merged
rkpattnaik780 merged 1 commit intomainfrom
topic_create_fix
Oct 28, 2021
Merged

fix(kafka topic create): erroneous flag validation#1258
rkpattnaik780 merged 1 commit intomainfrom
topic_create_fix

Conversation

@rkpattnaik780
Copy link
Contributor

Quick fix for rhoas kafka topic create that wasn't allowing user to create topic with default flag.

Verification Steps

  1. rhoas kafka topic create --name topic_1
  2. This should successfully create the topic

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation change
  • Other (please specify)

Checklist

  • Documentation added for the feature
  • CI and all relevant tests are passing
  • Code Review completed
  • Verified independently by reviewer

@rkpattnaik780
Copy link
Contributor Author

Maybe we can think of a better way to handle null string for output.

@craicoverflow
Copy link
Contributor

Is this bug currently in the latest released version that we need a quick fix?

Maybe we can think of a better way to handle null string for output.

The root of this problem is now that we have a "common" flag set, it assumes the default value to be "", but this will not always be the case.

The best thing to do here is to revert to not use the common flag helper for output for this command, but still use the new common description.

Longer term it would be nice to have a flag builder built into the common flag set, something like:

flags.AddOutput(&opts.output).DefaultValue("json").Required()

Would you mind reviewing to see if any other commands are affected by this?

@rkpattnaik780
Copy link
Contributor Author

Is this bug currently in the latest released version that we need a quick fix?

No, it is not there in the release

The best thing to do here is to revert to not use the common flag helper for output for this command, but still use the new common description.

Okay

Longer term it would be nice to have a flag builder built into the common flag set, something like:

flags.AddOutput(&opts.output).DefaultValue("json").Required()

Would you mind reviewing to see if any other commands are affected by this?

Similar occurrences have been enclosed within if statement checking output != "".

@rkpattnaik780
Copy link
Contributor Author

Sticking to if check as it is there for majority of commands.

@rkpattnaik780 rkpattnaik780 merged commit e56d443 into main Oct 28, 2021
@rkpattnaik780 rkpattnaik780 deleted the topic_create_fix branch October 28, 2021 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants