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

Adding options_description class method to base_manager #15799

Merged

Conversation

enoodle
Copy link

@enoodle enoodle commented Aug 13, 2017

This is a metadata function that should return the description of the data that is stored in the options field (added in ManageIQ/manageiq-schema#23)

requested here: ManageIQ/manageiq-api#3 (comment)

Copy link
Member

@durandom durandom left a comment

Choose a reason for hiding this comment

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

straightforward.

So, from looking at the PRs provider_settings will return a description what to expect in options ?
Is provider_settings already used somewhere? If not, I'd rather call it options_description to make it a) more related to the options field and b) express that it only contains a description.
It would be also great to see an example in either a spec or a method comment or the git commit msg

@enoodle
Copy link
Author

enoodle commented Aug 15, 2017

@durandom

I'd rather call it options_description

I only used it in ManageIQ/manageiq-providers-kubernetes#45, and ManageIQ/manageiq-api#3. I can change it in both places.

@durandom
Copy link
Member

Oh, so there are already proxy_settings and advanced_settings.

If thats only used in the kubernetes provider currently, I'd really rename that now before it spreads to other providers.

For the ansible tower credentials we've put the description into constants here

@jameswnl wdyt on naming those methods?

@enoodle enoodle force-pushed the base_manager_to_have_provider_settings branch from 16ee22d to 8a33241 Compare August 15, 2017 09:05
@enoodle enoodle changed the title Adding provider_settings class method to base_manager Adding options_description class method to base_manager Aug 15, 2017
@enoodle enoodle changed the title Adding options_description class method to base_manager Adding options_description class method to base_manager Aug 15, 2017
@enoodle
Copy link
Author

enoodle commented Aug 15, 2017

@durandom I made this change in all the places that used this function.

@durandom
Copy link
Member

@jameswnl @jntullo could you point out how the API and UI accesses the option descriptions? I'm more a fan of having actual methods than accessing constants. But we should indeed align the approaches as long as we only have ansible and kubernetes using this.

@jameswnl
Copy link
Contributor

API responds to OPTIONS call and return API_OPTIONS of credentials. See the spec here

@jntullo can correct/provide more on this

@durandom
Copy link
Member

ok, so what do you think of streamlining this via a options_description method? Or some better name?

roping in @imtayadeway as well

@enoodle
Copy link
Author

enoodle commented Aug 17, 2017

@durandom If you were asking me then I think options_description is a fitting name for this function.

@enoodle
Copy link
Author

enoodle commented Aug 21, 2017

ping @durandom @imtayadeway @jameswnl do we have a decision? Do we want it in a constant or a function?

@jntullo
Copy link

jntullo commented Aug 21, 2017

@enoodle a function seems fine to me.

@enoodle
Copy link
Author

enoodle commented Aug 21, 2017

Then we can merge this and ManageIQ/manageiq-providers-kubernetes#91 to proceed with ManageIQ/manageiq-api#3

Copy link
Member

@durandom durandom left a comment

Choose a reason for hiding this comment

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

I think you'll want to remove the yarn.lock file.
Also, either in a comment above the method I would shortly point out what it's used for. I know that we dont do this a lot, but it's never too late to start.
At least for those base methods in base managers which are supposed to be overwritten

@enoodle enoodle force-pushed the base_manager_to_have_provider_settings branch from 8a33241 to b7aa783 Compare August 24, 2017 14:01
@enoodle
Copy link
Author

enoodle commented Aug 24, 2017

@durandom PTAL

@miq-bot
Copy link
Member

miq-bot commented Aug 24, 2017

Checked commit enoodle@b7aa783 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

Copy link
Member

@durandom durandom left a comment

Choose a reason for hiding this comment

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

@miq-bot assign @agrare
@agrare pls merge once green

@agrare agrare merged commit 9e75dc4 into ManageIQ:master Aug 24, 2017
@agrare agrare added this to the Sprint 68 Ending Sep 4, 2017 milestone Aug 24, 2017
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