-
Notifications
You must be signed in to change notification settings - Fork 54
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
New Tower credential types #9
Conversation
@miq-bot remove_label wip |
hey @durandom - Can you please review this? Danke! |
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.
LGTM 👍
some question, but minor
@@ -0,0 +1,49 @@ | |||
module ManageIQ::Providers::AnsibleTower::Shared::AutomationManager::OpenstackCredential | |||
extend ActiveSupport::Concern |
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 see you are following the pattern in the other credential modules, but why is AS::Concern
used here?
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.
Done!
@@ -80,6 +80,37 @@ class PopulateTower | |||
end | |||
|
|||
def create_dataset | |||
|
|||
ssh_key_data = <<~HEREDOC | |||
-----BEGIN RSA PRIVATE KEY----- |
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 sharing you private ssh key 😈
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.
you are welcome. generated for this particular purpose only 😄
@@ -100,12 +100,12 @@ | |||
def assert_counts | |||
expect(Provider.count).to eq(1) | |||
expect(automation_manager).to have_attributes(:api_version => "3.0.1") | |||
expect(automation_manager.configured_systems.count).to eq(130) |
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.
these counts are a pain.
in aws we changed this to get the counts straight from the API and just assert they are equal to objects counts in the DB.
You would access the API easily via automation_manager.provider.connect.hosts.count
Maybe add a safe trap, that the numbers are non-zero
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.
yeah, was actually considering this. but thinking of using the Faraday
direct call to avoid depending on the Ansible Tower Gem
. What do you think?
automation_manager.provider.connect.hosts.count
would be much more straightforward.
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.
@durandom let's keep this as is for now. There's some strange issue with the Tower we have been using to perform this test.
(The reported count of hosts is more than the actual hosts returned)
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.
The reported count of hosts is more than the actual hosts returned
🤔
If you have some spare time 😆 maybe you want to add a docker run
for setting up a tower and populating it?
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 did and ended up aborting it because it needs enterprise license to support survey spec.
Some comments on commits https://github.com/jameswnl/manageiq-providers-ansible_tower/compare/64aaae7bd1c7d1f5ff2019207ded963da351aedf~...80c24cecbf9653bd72e82c6b9b1533547b6dc4ac spec/vcr_cassettes/manageiq/providers/ansible_tower/automation_manager/refresher.yml
|
Checked commits https://github.com/jameswnl/manageiq-providers-ansible_tower/compare/64aaae7bd1c7d1f5ff2019207ded963da351aedf~...80c24cecbf9653bd72e82c6b9b1533547b6dc4ac with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 app/models/manageiq/providers/ansible_tower/shared/automation_manager/azure_credential.rb
app/models/manageiq/providers/ansible_tower/shared/automation_manager/google_credential.rb
app/models/manageiq/providers/ansible_tower/shared/automation_manager/network_credential.rb
app/models/manageiq/providers/ansible_tower/shared/automation_manager/openstack_credential.rb
app/models/manageiq/providers/ansible_tower/shared/automation_manager/rackspace_credential.rb
app/models/manageiq/providers/ansible_tower/shared/automation_manager/satellite6_credential.rb
lib/tasks_private/spec_helper.rake
|
@miq-bot add_labels fine/yes |
@jameswnl Is there a BZ for this? Can you please create if it doesn't exist? |
Backported to Fine via ManageIQ/manageiq#15780 |
Make MIQ's support to be on par with what Tower supports.
https://www.pivotaltracker.com/story/show/148234657
Created https://bugzilla.redhat.com/show_bug.cgi?id=1478898 for backporting to
fine