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

Allow additional provider parameters to be specified on create #279

Merged
merged 1 commit into from
Feb 14, 2018

Conversation

jntullo
Copy link

@jntullo jntullo commented Jan 11, 2018

This update will allow individual providers to have more control over the parameters that are allowed when creating a new manager. This will also be a good transition starting point to move a lot of the validation inside of the providers themselves, rather than having it all within the API controller.

As an example, the Azure provider would like users to provide azure_tenant_id, which is not possible under the current API validation. By updating .api_allowed_attributes on the Azure::CloudManager to return azure_tenant_id, users will be allowed to specify that attribute for Azure Cloud Managers.

@miq-bot add_label wip, enhancement
cc: @juliancheal @bronaghs

Dependent on ManageIQ/manageiq#16802

https://bugzilla.redhat.com/show_bug.cgi?id=1669600

@juliancheal
Copy link
Member

Is this ready to be un-wipped?

@jntullo
Copy link
Author

jntullo commented Jan 12, 2018

@juliancheal will unwip when ManageIQ/manageiq#16802 is merged

Copy link
Contributor

@cben cben left a comment

Choose a reason for hiding this comment

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

So this allows writing azure_tenant_id but does this also allow reading it back?

https://github.com/juliancheal/manageiq-providers-azure/blob/1394bf82a4/app/models/manageiq/providers/azure/cloud_manager.rb#L21

alias_attribute :azure_tenant_id, :uid_ems

So perhaps it'll get sent back under uid_ems key on read (?)
BTW, why do we need the alias on write rather than telling users to put tenant in uid_ems? Because that's just an implementation detail? If we want to hide it on write, let's also hide it on read IMHO.

In general, providers read/write has various asymmetries (#26), I'd like to reduce not increase them...

@juliancheal
Copy link
Member

Good point @cben I'll check it's read as azure_tenant_id too.

The field that the user would know from Azure documentation is tenant_id, but as that clashes with ManageIQ tenants we've had to change what value we store it in.

@bronaghs
Copy link

@cben Why do you believe the user cannot read back the azure_tenant_id? Do you mean like this? :

ManageIQ::Providers::Azure::CloudManager.first.azure_tenant_id


expected = {
"results" => [a_hash_including("uid_ems" => tenant.id.to_s, "name" => "sample azure provider")]
}

Choose a reason for hiding this comment

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

@jntullo - does the results hash mirror the database columns? Is that why the first hash key is uid_ems and not azure_tenant_id

Copy link
Author

Choose a reason for hiding this comment

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

@bronaghs yeah, it is all of the physical attributes

@cben
Copy link
Contributor

cben commented Jan 15, 2018 via email

@bronaghs
Copy link

Ah @cben, thanks for clarifying - I touched on the same point in my review comment above
I, too, think we should read back azure_tenant_id.

@jntullo
Copy link
Author

jntullo commented Jan 16, 2018

@bronaghs I did some thinking on this and now that I see this is a similar issue in openstack (where keystone_v3_domain_id is an alias of :uid_ems), I am thinking it may be a good idea to consider a serializer for these objects, so that they can get read back the same way. Thoughts? @juliancheal @cben @abellotti

snecklifter added a commit to snecklifter/ansible that referenced this pull request Jan 24, 2018
Adds the capability to create OpenStack providers. Because one
of the parameters, keystone_v3_domain_id, is not currently
exposed, it is added as an alias of azure_tenant_id which currently
maps to uid_ems. Work is in progress to fix this.

ManageIQ/manageiq#16802
ManageIQ/manageiq-api#279
ManageIQ/manageiq-providers-openstack#196
snecklifter added a commit to snecklifter/ansible that referenced this pull request Jan 26, 2018
Adds the capability to create OpenStack providers. Because one
of the parameters, keystone_v3_domain_id, is not currently
exposed, it is added as an alias of azure_tenant_id which currently
maps to uid_ems. Work is in progress to fix this.

ManageIQ/manageiq#16802
ManageIQ/manageiq-api#279
ManageIQ/manageiq-providers-openstack#196
Lujeni pushed a commit to Lujeni/ansible that referenced this pull request Feb 1, 2018
Adds the capability to create OpenStack providers. Because one
of the parameters, keystone_v3_domain_id, is not currently
exposed, it is added as an alias of azure_tenant_id which currently
maps to uid_ems. Work is in progress to fix this.

ManageIQ/manageiq#16802
ManageIQ/manageiq-api#279
ManageIQ/manageiq-providers-openstack#196
@jntullo jntullo changed the title [WIP] Allow additional provider parameters to be specified on create Allow additional provider parameters to be specified on create Feb 7, 2018
@jntullo jntullo removed the wip label Feb 7, 2018
This update will allow providers to have more control over the parameters that are allowed when creating a new manager. Currently, only those columns for a provider are allowed. This will also be a good transition starting point to put more of the validation inside of the providers themselves, rather than store it all within the API controller.
@miq-bot
Copy link
Member

miq-bot commented Feb 7, 2018

Checked commit jntullo@feeaa6c with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@abellotti abellotti added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 14, 2018
@abellotti
Copy link
Member

LGTM!! Thanks @jntullo for this enhancement. 👍

@abellotti abellotti merged commit 6983681 into ManageIQ:master Feb 14, 2018
@miq-bot miq-bot changed the title Allow additional provider parameters to be specified on create [WIP] Allow additional provider parameters to be specified on create Feb 14, 2018
@miq-bot miq-bot added the wip label Feb 14, 2018
@jntullo jntullo deleted the allow_provider_attributes branch February 14, 2018 19:36
@abellotti abellotti changed the title [WIP] Allow additional provider parameters to be specified on create Allow additional provider parameters to be specified on create Feb 27, 2018
@abellotti abellotti removed the wip label Feb 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants