Repository navigation
Fixes #38957 - Add descriptions to model create/update options - #661
anandjambhulkar wants to merge 1 commit into
Conversation
ofedoren
left a comment
ofedoren
left a comment
There was a problem hiding this comment.
Thanks, @anandjambhulkar, but I'm afraid I'd reject this PR, see the comment inline.
But, instead of closing, I'll keep it open with the requested changes status, so we can see if there will be something needed even if the API side is fixed.
| success_message _("Hardware model created.") | ||
| failure_message _("Could not create the hardware model") | ||
|
|
||
| option '--name', 'NAME', _('Name of the hardware model'), :required => true |
There was a problem hiding this comment.
There are few issues with this:
- We don't create options manually unless it's actually needed (e.g. to override automatically created one).
NAMEshould be type instead of duplicating already known information, see other options in--helpoutput.- If there are missing descriptions, but the options themselves are there, then it should be fixed on the API side: https://github.com/theforeman/foreman/blob/develop/app/controllers/api/v2/models_controller.rb#L24-L27
There was a problem hiding this comment.
Hi @ofedoren. Thanks for reviewing my PR, and suggesting where the problem lies. I can perform some modifications in the above stated file to fix the issue. Would that be fine? Please let me know. Thanks.
There was a problem hiding this comment.
Hi @anandjambhulkar , ideally we don't even need to change this file at all. As I've mentioned those options should be created by hammer automatically based on the API documentation. So, to get the descriptions, we should first provide them on the API side: https://github.com/theforeman/foreman/blob/develop/app/controllers/api/v2/models_controller.rb#L24-L27.
So, as a first step I would do is to create a PR against Foreman, add those descriptions in the mentioned file and see if hammer is able to fetch and show them automatically (without this PR).
There was a problem hiding this comment.
Hello @ofedoren. Thanks for your suggestions. I have made modifications to the above file that you have suggested, and tested it on my Satellite Server, and it works successfully. Thus, as per your guidance, I have created another PR against Foreman, whose URL is as follows - theforeman/foreman#11221. Let me know if I should close this current PR or not.
There was a problem hiding this comment.
Perfect, thanks. I've approved the theforeman/foreman#11221, so we no longer need this PR at all :) Would you mind closing it?
There was a problem hiding this comment.
Yes, definitely @ofedoren. I will go ahead and close out this PR immediately.
|
Closing out this PR as the following PR related to this has been approved and merged - theforeman/foreman#11221. |
Summary
hammer model createoptionshammer model updateoptions, including--idand--new-nameRelated issue
Fixes https://projects.theforeman.org/issues/38957
Test plan
hammer model create --helpshows descriptions for--name,--info,--vendor-class,--hardware-modelhammer model update --helpshows descriptions for--id,--name,--new-name, and other model fields