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 #30384 - Fix ansible secrets to support effective_user_password #350

Merged
merged 3 commits into from Aug 10, 2020

Conversation

patilsuraj767
Copy link
Contributor

No description provided.

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

@patilsuraj767
Copy link
Contributor Author

This change is required because of - theforeman/foreman_remote_execution#508

Comment on lines 45 to 47
def rex_ssh_password(host)
host_setting(host, 'remote_execution_ssh_password')
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to do this? The ssh password can still be different from the effective user one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I should only change sudo_pass. Got confused in ansible_ssh_pass and ansible_su_pass. :P
But I think we should change ansible variable from "ansible_sudo_pass" to "ansible_become_password", it will help to deal with both sudo and su user method.
I will add the ansible_ssh_pass again.

@adamruzicka
Copy link
Contributor

The changes in rex will be released in 4.0.0, could you bump the versions in gemspec here?

Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

Just a nit.

test/unit/ansible_provider_test.rb Outdated Show resolved Hide resolved
def rex_sudo_password(host)
host_setting(host, 'remote_execution_sudo_password')
def rex_effective_user_password(host)
host_setting(host, 'remote_execution_effective_user_password')
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this imply a migration of old settings is necessary?
We need migration to rename setting like this:

Setting.where(name: 'remote_execution_sudo_password').update_all(name: 'remote_execution_effective_user_password')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ezr-ondrej Migration is written in this PR - theforeman/foreman_remote_execution#508

Copy link
Member

Choose a reason for hiding this comment

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

Right 👍 Thanks for the pointer, missed that out. In that case no more picks.

@ezr-ondrej
Copy link
Member

@adamruzicka this should get in before release for foreman 2.2, right?

@adamruzicka
Copy link
Contributor

Ideally, yes. The changes in rex will go out in 4.0.0, which will be the first >=2.2 release

Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

Let's get this in then, thanks @patilsuraj767 !

@ezr-ondrej ezr-ondrej merged commit cf16610 into theforeman:master Aug 10, 2020
@tbrisker
Copy link
Member

Looks like one of these tests is now failing: https://ci.theforeman.org/job/test_plugin_matrix/2441/

@adamruzicka
Copy link
Contributor

Being handled here #360

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