Skip to content

Conversation

@SamiSousa
Copy link
Contributor

  • Change validation flag to use dashes instead of underscores

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

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 13, 2019
@kevinrizza
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

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

@kevinrizza
Copy link
Member

@SamiSousa Looks like the tests failed.

@MartinBasti
Copy link
Contributor

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

@SamiSousa
Copy link
Contributor Author

@MartinBasti Removed a newline based on your comment.

@MartinBasti
Copy link
Contributor

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

You have add tests or lower coverage limit.

@SamiSousa SamiSousa force-pushed the ui-validate-flag branch 2 times, most recently from 565666f to 6ac2c5d Compare April 1, 2019 13:59
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2019
@SamiSousa
Copy link
Contributor Author

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

@kevinrizza
Copy link
Member

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

- Add ui-validate-io flag with dashes
- Add comment to ui_validate_io as deprecated

This follows the conventions set by other CLI tools
@SamiSousa SamiSousa force-pushed the ui-validate-flag branch from 6ac2c5d to db0a653 Compare May 3, 2019 20:09
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2019
@SamiSousa
Copy link
Contributor Author

@kevinrizza @JEREMYLINLIN Rebased based on latest master, please take a look 🙂

Copy link
Member

@jeremy-wl jeremy-wl left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants