search

operator-framework / operator-courier

Build, verify and push operators
Apache License 2.0
41 stars 53 forks source link

[cli] Fix: Use dashes in ui validate flag #69

Open SamiSousa opened 5 years ago

SamiSousa commented 5 years ago

This follows the conventions set by other CLI tools Replaces part of #65

kevinrizza commented 5 years ago

@SamiSousa So I see the appeal of making this change, but my concern is that this is already in the wild and being used. Making a breaking change like this has to be a major version bump, but I'm not sure this change is a good enough reason to introduce breaking changes to the cli.

MartinBasti commented 5 years ago

I suggest:

parser.add_argument(
    '--ui-validate-io',
    dest='ui_validate_io',
    help='Validate bundle for operatorhub.io UI',
    action='store_true')
parser.add_argument(
   # DEPRECATED; BW compatibility
    '--ui_validate_io',
    dest='ui_validate_io'
    help=argparse.SUPPRESS,
    action='store_true')
kevinrizza commented 5 years ago

I suggest:

parser.add_argument(
    '--ui-validate-io',
    dest='ui_validate_io',
    help='Validate bundle for operatorhub.io UI',
    action='store_true')
parser.add_argument(
   # DEPRECATED; BW compatibility
    '--ui_validate_io',
    dest='ui_validate_io'
    help=argparse.SUPPRESS,
    action='store_true')

Great suggestion.

@SamiSousa Can you update your pr this way?

SamiSousa commented 5 years ago

Thanks @MartinBasti ! Applied your suggestion, please take a look

kevinrizza commented 5 years ago

@SamiSousa Looks like the tests failed.

MartinBasti commented 5 years ago

Coverage decreased under a failure threshold because commit added a new line.

SamiSousa commented 5 years ago

@MartinBasti Removed a newline based on your comment.

MartinBasti commented 5 years ago

@SamiSousa oh sorry, I meant new line from code execution POW, not one line literally. :)

You have add tests or lower coverage limit.

SamiSousa commented 5 years ago

@kevinrizza @jeremylinlin If there's an interest in this change, let me know so I can rebase. Otherwise, I'll close this PR.

kevinrizza commented 5 years ago

@SamiSousa Feel free to update, it looks like a good change. once you do please @ me and Wenlin so we can review again

SamiSousa commented 5 years ago

@kevinrizza @jeremylinlin Rebased based on latest master, please take a look :slightly_smiling_face: