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

Add api_allowed_attributes to ExtManagementSystem and Provider #16802

Merged
merged 1 commit into from
Feb 7, 2018

Conversation

jntullo
Copy link

@jntullo jntullo commented Jan 11, 2018

Required for ManageIQ/manageiq-api#279

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.

cc: @bronaghs @juliancheal - let me know your thoughts on this approach

@miq-bot enhancement, providers

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

@miq-bot
Copy link
Member

miq-bot commented Jan 11, 2018

@jntullo unrecognized command 'enhancement', ignoring...

Accepted commands are: add_label, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

@jntullo
Copy link
Author

jntullo commented Jan 11, 2018

@miq-bot add_label enhancement, providers

@juliancheal
Copy link
Member

I like it, so if we don't do anything it's business as usual, but overriding this in the provider gives us more attributes. Yay!

@juliancheal
Copy link
Member

Do we need to update the API docs too?

@jntullo
Copy link
Author

jntullo commented Jan 11, 2018

@juliancheal I'll update ManageIQ/manageiq-documentation#521 where I have the provider examples

@juliancheal
Copy link
Member

@jntullo great thanks, was just trying to find where to do it.

@jntullo jntullo changed the title Add API_ALLOWED_ATTRIBUTES constant to ExtManagementSystem and Provider Add api_allowed_attributes to ExtManagementSystem and Provider Jan 11, 2018
@@ -27,6 +27,10 @@ def self.supported_types_and_descriptions_hash
end
end

def self.api_allowed_attributes
%w()
Copy link
Member

Choose a reason for hiding this comment

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

Ah I see you've changed it to a method. You'll need to update the commit message, as it still mentions it being a constant.

Copy link
Author

Choose a reason for hiding this comment

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

@juliancheal updated ✨

@juliancheal
Copy link
Member

LGTM 👍 just need to fix the Rubocops 🍰

The addition of the api_allowed_attributes method will allow individual providers to have better control over the attributes that are allowed on a provider, including aliases that are more clear to the user.
@juliancheal
Copy link
Member

@miq-bot assign @bronaghs

@miq-bot
Copy link
Member

miq-bot commented Jan 12, 2018

Checked commit jntullo@37a0b23 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. ⭐

@juliancheal
Copy link
Member

@miq-bot add_label gaprindashvili/yes

@djberg96
Copy link
Contributor

👍

@jntullo
Copy link
Author

jntullo commented Jan 12, 2018

@miq-bot assign @gtanzillo

@miq-bot miq-bot assigned gtanzillo and unassigned bronaghs Jan 12, 2018
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
@juliancheal
Copy link
Member

Ping @gtanzillo Is this good to be merged?

@gtanzillo gtanzillo added this to the Sprint 79 Ending Feb 12, 2018 milestone Feb 7, 2018
@gtanzillo gtanzillo merged commit 6b8a2fd into ManageIQ:master Feb 7, 2018
@juliancheal
Copy link
Member

@gtanzillo thanks Gregg!!! 🍰

And thanks @jntullo for working on this, now onto all the other PRs!

@simaishi
Copy link
Contributor

simaishi commented Mar 7, 2018

@jntullo ManageIQ/manageiq-api#279 isn't marked as gaprindashvili/yes, so I assume this doesn't need to go to gaprindashvili either?

@jntullo
Copy link
Author

jntullo commented Mar 7, 2018

@juliancheal @bronaghs is this needed in g? Then I'll mark the api one as such

@bronaghs
Copy link

bronaghs commented Mar 7, 2018

Hi @jntullo - I thought this was owned by the API. Was there a BZ? Were the related PRs backported?

@simaishi
Copy link
Contributor

simaishi commented Mar 8, 2018

Marking as conflict for now..

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.

8 participants