Skip to content
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 #23394 - Unable to set effective user for job templates #361

Merged
merged 1 commit into from Jul 10, 2018
Merged

Conversation

ik5
Copy link

@ik5 ik5 commented Jun 27, 2018

There was a problem with the API structure, it didn't work since day one.
The ssh sub entry was not recognized by strong params, making the ssh
object (hash) and children not to be accessible.

This PR fixes the issue, but "breaks" the bad API documentation that existed
before.

@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • commit message for c8a263f is not wrapped at 72nd column
  • commit message for c8a263f is not wrapped at 72nd column
  • commit message for c8a263f is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Works as expected, left one inline comment, but I don't feel that strongly about it, more opinions welcome.

param :current_user, :bool, :desc => N_('Whether the current user login should be used as the effective user')
end
param :effective_user_attributes, Hash, :desc => N_('Effective user options') do
param :value, String, :desc => N_('What user should be used to run the script (using sudo-like mechanisms)'), :allowed_nil => true
Copy link
Contributor

Choose a reason for hiding this comment

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

Having this called value makes sense in the API context, but in hammer having an option called value, which is not really a value of the object you're manipulating with, feels weird

hammer job-template create \
    --file my_template.erb \
    --name my_template \
    --job-category Misc \
    --provider-type SSH \
    --value nobody

Maybe renaming this either in the apidoc or in hammer would lead to a better user experience

Copy link
Author

Choose a reason for hiding this comment

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

I do agree with you, but I don't think it should be on this PR, this one just fixes an issue, another PR can be on better naming things

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so it was called value even before, but it didn't work. Guess I'm ok with that

Copy link
Author

Choose a reason for hiding this comment

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

the ssh hash was inaccessible because it does not exists anywhere else, so I fixed that.

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Also could we get a test or two?

There was a problem with the API structure, it didn't work since day
one.
The `ssh` sub entry was not recognized by `strong params`, making the
`ssh` object (hash) and children not to be accessible.

This PR fixes the issue, but "breaks" the bad API documentation that
existed before.
@ik5
Copy link
Author

ik5 commented Jul 2, 2018

done :)

@adamruzicka adamruzicka merged commit 019407c into theforeman:master Jul 10, 2018
@adamruzicka
Copy link
Contributor

Thanks @ik5 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants