-
Notifications
You must be signed in to change notification settings - Fork 35
Fixes #38402 - Do not use humanized input #62
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
Conversation
bb60cb1 to
b7333e0
Compare
|
This feels a bit odd, have you looked at where it comes from? At a glance it seems to be rather katello-specific |
|
Yes, it appears that humanized input which is formatted as a hash comes only from actions originated in katello. |
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.
I'd say it should be handled at least in hammer-cli-katello, but we could fix that here.
I've noticed that task["humanized"]["action"] content is not humanized, but to rather build a humanized output, which we don't do. On the other hand task["action"] seems to already contain humanized version. Should we maybe use that instead?
UPD: It seems we have quite inconsistent responses :/ It would help with Katello ones and won't show weird hashes in the output, but it still won't be perfectly humanized output. It needs one to go over those Actions and make them return consistent values.
I think the output in task["action"] for katello actions is fine, and also from my observations it is consistent with how the action is displayed in the UI, so users are used to seeing it. I am not sure if there is any action item for me, I would prefer not to deviate from the UI output if people are not complaining about it. |
Sure thing, if this field in hammer is for a human to read (not automate on) then it should be as similar as it is in the UI. What I meant is that even in |
|
@ofedoren Can I get an example of such action? I am no longer sure that I am referring to the same type of action as you are. |
|
@adamlazik1, I apologize, it seems in the logs I was sometimes looking at the |
Hmm. I thought that I saw some differences between humanized action and action that would be considered an improvement, but right now I cannot find such instances. I guess we can drop it and see if anyone complains about it, although I doubt it. EDIT: On the other hand, there might be cases where humanized input could improve task['action'] which I do not know of. I think it would be safer to just leave it there and prevent it's use when it's a hash. I guess the bottom line is that I would rather not remove it, but if you think that it is beneficial I will do so. |
lib/hammer_cli_foreman_tasks/task.rb
Outdated
| module ActionField | ||
| def extend_data(task) | ||
| task["action"] = [task["humanized"]["action"], task["humanized"]["input"]].join(' ') | ||
| task["action"] = [task["humanized"]["action"], task["humanized"]["input"]].join(' ') if task["humanized"]["input"].is_a?(String) |
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.
It seems task["action"] is already being created from humanized attributes:
- hammer gets what is returned from .rabl: https://github.com/theforeman/foreman-tasks/blob/master/app/views/foreman_tasks/api/tasks/show.json.rabl#L5
actionon a Task model is defined to useto_label: https://github.com/theforeman/foreman-tasks/blob/master/app/models/foreman_tasks/task.rb#L237-L240to_labelcreates a humanized string: https://github.com/theforeman/foreman-tasks/blob/master/app/models/foreman_tasks/task.rb#L242-L253
Unless I'm missing something, we don't really need extend_data here at all.
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 pointing that out, I didn't realize that. I removed the ActionField module.
b7333e0 to
50f860d
Compare
ofedoren
left a comment
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, @adamlazik1 ! If it breaks something, it's my fault :)
|
@adamlazik1, one more thing though, could you please update the commit message to reflect the real change? |
Sometimes, humanized input is a composite array, and then the output
looks like this:
```
hammer task list --fields Action
...
Promote content_view {"text"=>"content view 'CV-1'", "link"=>"/content_views/...
Publish content_view {"text"=>"content view 'CV-1'", "link"=>"/content_views/...
Update content_view {"text"=>"content view 'CV-1'", "link"=>"/content_views/2...
Synchronize repository {"text"=>"repository 'Red Hat Ansible Engine 2 for RHE...
```
Using field["Action"] removes this issue with no apparent repercussions.
50f860d to
4a31045
Compare
|
@ofedoren my apologies, I forgot about that. Updated. |
Sometimes, humanized input is a composite array, and then the output
looks like this:
Using field["Action"] removes this issue with no apparent repercussions.