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.

Sign up for GitHub

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

Jump to bottom

Adds cloud credentials to AnsibleRunner (for EmbeddedAnsible) #18991

Merged
merged 7 commits into from
Jul 17, 2019
Merged

Adds cloud credentials to AnsibleRunner (for EmbeddedAnsible) #18991

merged 7 commits into from
Jul 17, 2019

Conversation

NickLaMuro
Copy link
Member

@NickLaMuro NickLaMuro commented Jul 17, 2019 •
edited
Loading

Adds the following credential types:

  • AmazonCredential
  • AzureCredential
  • GoogleCredential
  • OpenstackCredential
  • RhvCredential
  • VmwareCredential

Documented most of places where this is pulled from in awx in the code/specs.

TODO:

  • VmwareCredential

Steps for Testing/QA

TBD

That said, this definitely needed. I am just conirming stuff exists with these specs, and haven't tested that ansible-runner actually works with this changes.

@NickLaMuro
Copy link
Member Author

@miq-bot add_label changelog/yes, core, embedded ansible, enhancement

cc @carbonin @Fryguy

@NickLaMuro NickLaMuro force-pushed the ansible_runner_cloud_credentials branch from ab02e89 to 61cde3e Compare July 17, 2019 02:17
carbonin
carbonin reviewed Jul 17, 2019
View reviewed changes
lib/ansible/runner/credential/azure_credential.rb Outdated Show resolved Hide resolved
lib/ansible/runner/credential/google_credential.rb Outdated Show resolved Hide resolved
lib/ansible/runner/credential/openstack_credential.rb Outdated Show resolved Hide resolved
lib/ansible/runner/credential/azure_credential.rb Outdated Show resolved Hide resolved
lib/ansible/runner/credential/google_credential.rb Outdated Show resolved Hide resolved
lib/ansible/runner/credential/openstack_credential.rb Outdated Show resolved Hide resolved
lib/ansible/runner/credential/openstack_credential.rb Show resolved Hide resolved
lib/ansible/runner/credential/rhv_credential.rb Outdated Show resolved Hide resolved
lib/ansible/runner/credential/vmware_credential.rb Show resolved Hide resolved
@NickLaMuro NickLaMuro force-pushed the ansible_runner_cloud_credentials branch from 4ecdab9 to 63aae5b Compare July 17, 2019 15:51
@NickLaMuro
Copy link
Member Author

Okay, updated most of the original concerns, but this one:

#18991 (comment)

Is going to take a bit more of a larger rebase to do, so I will be working on that for a bit.

@carbonin
Copy link
Member

Which direction are you going to go with that @NickLaMuro ?

@NickLaMuro
Copy link
Member Author

@carbonin Oh, sorry, I wasn't clear. I am just going to go with your suggestion of write_config_file and have the subclasses determine in that method what files need to be written. So I need to add a commit to MachineCredential to do that and update the other three classes to use that, which will take a bit.

@NickLaMuro NickLaMuro force-pushed the ansible_runner_cloud_credentials branch from 63aae5b to cd94fcc Compare July 17, 2019 17:17
@NickLaMuro
Copy link
Member Author

@carbonin Okay, that wasn't as hard as I thought it was going to be. Added a new commit at the beginning, and everything after that follows that pattern.

NickLaMuro
NickLaMuro commented Jul 17, 2019
View reviewed changes
lib/ansible/runner/credential/rhv_credential.rb
end

def ovirt_ini_file
File.join(base_dir, "ovirt.ini")
Copy link
Member Author

Choose a reason for hiding this comment

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

Quick note here: I changed this from rhv_credentials (what it was in the previous version) since this is what the file is normally called:

https://github.com/openshift/openshift-ansible-contrib/blob/d16d2f73/reference-architecture/rhv-ansible/inventory/ovirt.ini.example#L21-L34

@carbonin carbonin self-assigned this Jul 17, 2019
@carbonin
Copy link
Member

Looks good to me. Is this still WIP @NickLaMuro ?

Fryguy
Fryguy approved these changes Jul 17, 2019
View reviewed changes
Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

LGTM, let's just get the TODO comments out of there as we've made decisions

@NickLaMuro
Copy link
Member Author

... let's just get the TODO comments out of there as we've made decisions

@Fryguy I feel like this hasn't been done at all. There have been comments say "we don't currently support in our dialogs" (and I agree), but there haven't seen an out right "we are never going to support this" from either of you.

That said, I am not against merging as is at all, but the reason I put these TODO's in there in the first places was they markers of "things ansible/awx supports that we don't yet". And I feel that at some point some one is going to request this feature, and these comments are meant to be notes about where this stuff lives in tower in case someone else (besides us) has to work in this code.

Should I change these to "Note: ..." comments instead? Maybe add a bit more context to each as to why we aren't supporting them?

@carbonin
Copy link
Member

I think he's fine with the references to the awx codebase, but TODOs for things that we never supported regardless of backend implementation don't belong here, they belong in an RFE in a bug tracker.

@NickLaMuro NickLaMuro force-pushed the ansible_runner_cloud_credentials branch from cd94fcc to 5d9652d Compare July 17, 2019 21:28
@NickLaMuro NickLaMuro changed the title [WIP] Adds cloud credentials to AnsibleRunner (for EmbeddedAnsible) Adds cloud credentials to AnsibleRunner (for EmbeddedAnsible) Jul 17, 2019
@miq-bot miq-bot removed the wip label Jul 17, 2019
@NickLaMuro
[ansible_runner] write_password_file -> write_config_files
80f39cf 
Changes `Ansible::Runner::Credential#write_password_file` to
`Ansible::Runner::Credential#write_config_file` since this allows it to
be more generic and have this method support multiple file types.

For example, machine credential not only writes a `env/password` file,
but also a `env/ssh_key` file.  But in some of the cloud credentials,
there are cloud specific files that are generated, so this allows the
same interface to be reused, and then the logic to be implemented in
private subclass specific methods.

Machine credential has been updated to show how that implementation can
be used.
@NickLaMuro
[ansible_runner] Add AmazonCredential
f6e3a82
@NickLaMuro
[ansible_runner] Add AzureCredential
7577172
@NickLaMuro
[ansible_runner] Add GoogleCredential
ef4a40a
@NickLaMuro
[ansible_runner] Add OpenstackCredential
9fcb382
@NickLaMuro NickLaMuro force-pushed the ansible_runner_cloud_credentials branch from 5d9652d to bf698f8 Compare July 17, 2019 21:32
@miq-bot
Copy link
Member

miq-bot commented Jul 17, 2019

Some comments on commits NickLaMuro/manageiq@80f39cf~...bf698f8

lib/ansible/runner/credential/machine_credential.rb

  • ⚠️ - 34 - Detected pp. Remove all debugging statements.
  • ⚠️ - 35 - Detected pp. Remove all debugging statements.

@miq-bot
Copy link
Member

miq-bot commented Jul 17, 2019

Checked commits NickLaMuro/manageiq@80f39cf~...bf698f8 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
18 files checked, 0 offenses detected
Everything looks fine. 🏆

@carbonin carbonin merged commit e120e56 into ManageIQ:master Jul 17, 2019
@carbonin carbonin added this to the Sprint 116 Ending Jul 22, 2019 milestone Jul 17, 2019
@Fryguy
Copy link
Member

Fryguy commented Jul 18, 2019

Yeah I was more commenting about the literal TODOs that I interpreted as net-new features where the decision was to not implement them until later. The comments with source reference are fine as is

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbonin carbonin carbonin approved these changes

@Fryguy Fryguy Fryguy approved these changes

Assignees

@carbonin carbonin

Labels
changelog/yes core/embedded ansible core enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NickLaMuro @carbonin @miq-bot @Fryguy