Fixes #19759 - Add options to skip publish and promote #159
Conversation
|
Something makes me think this should be |
|
👍 from me. @thomasmckay any objection? |
lib/hammer_cli_csv/content_views.rb
Outdated
| @@ -9,6 +9,9 @@ class ContentViewsCommand < BaseCommand | |||
| command_name 'content-views' | |||
| desc 'import or export content-views' | |||
|
|
|||
| option %w(--no-publish), :flag, _('Skip publishing content views on import') | |||
| option %w(--no-promote), :flag, _('Skip promoting content views on import') | |||
There was a problem hiding this comment.
Perhaps there should be a small bit of validation here; if --export is given, these options should be disallowed. Additionally, you might consider having a validation for --no-publish requires --no-promote. If you negate the options, you'd require --publish if you have --promote. Of course, you could also change the option description to be option %w(--promote), :flag, _('Publish and promote content views on import'); maybe that's the better solution.
There was a problem hiding this comment.
The --export validation sounds like a good idea.
I wasn't thinking that --no-publish requires --no-promote. IOW, if you pass in --no-publish it automatically doesn't promote. You can still pass in --no-promote but you don't have to.
Similarly, I was thinking if you call --promote, it would automatically publish. I was thinking this would be more convenient for users but I suppose maybe explicit is better?
There was a problem hiding this comment.
Ah, I overlooked your last sentence. I think I will go with updating the descriptions.
There was a problem hiding this comment.
The validation is good to do, if possible. I would suggest that the options follow the hammer pattern of --[no-]publish
https://github.com/Katello/hammer-cli-csv/blob/master/lib/hammer_cli_csv/products.rb#L7
| @@ -25,7 +25,7 @@ http_interactions: | |||
| message: Not Found | |||
There was a problem hiding this comment.
I don't think you need to commit this file.
|
+1 to defaulting to not publishing and promoting |
daviddavis
force-pushed
the
19759
branch
6 times, most recently
from
dcdc4a8
to
3d84b6a
Compare
|
@akofink @thomasmckay think I addressed all the feedback. thanks for the reviews/comments. |
|
It works as expected. Just one quirk: the publish If it's not important or too difficult to fix, that's fine. It's a pretty small gripe. ACK https://gist.github.com/akofink/773af789833bbce87a4692768220ec79 |
|
@akofink should be fixed now. |
No description provided.