-
Notifications
You must be signed in to change notification settings - Fork 99
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #35219 - Add options for table preferences CLI #603
Conversation
a266cfd
to
a7dc76d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @pkoprda, tested with current theforeman/foreman#9305 :)
Few requests inline, mostly cosmetic ones. Also, after theforeman/foreman#9305 is merged, we would need to generate new foreman_api.json
for test/data
and write some tests to cover the new commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @pkoprda, just one last bit:
class UpdateCommand < HammerCLIForeman::UpdateCommand | ||
success_message _('Table preference updated.') | ||
failure_message _('Could not update table preference') | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I forgot to mention that we need to remove automatically added --new-name
option to not confuse the users. You can do it by removing searchables for this resource (we don't need them anyway). Just add a new item similarly to this: https://github.com/theforeman/hammer-cli-foreman/blob/master/lib/hammer_cli_foreman/id_resolver.rb#L69
:compute_attribute => [],
:table_preference => []
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for suggestion, now it should be good
6592dff
to
d9dd260
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @pkoprda!
It's so good it hurts 🤌
User should be able to specify the columns that will see through table preferences CLI commands
Depends on #35212