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

[WIP] Settings for providers #10944

Merged
merged 1 commit into from
Dec 9, 2016
Merged

Conversation

durandom
Copy link
Member

@durandom durandom commented Sep 1, 2016

Purpose or Intent

support for having the same settings.yml files in the config and environments directory for provider gems

@miq-bot add_label pluggable providers, wip
@miq-bot assign Fryguy

cc @blomquisg

@@ -2,8 +2,8 @@ module VmOrTemplate::Operations::Relocation
extend ActiveSupport::Concern

included do
supports_not :live_migrate, :reason => _("Operation not supported.")
supports_not :evacuate, :reason => _("Operation not supported.")
supports_not :live_migrate, :reason => "Operation not supported."
Copy link
Member Author

Choose a reason for hiding this comment

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

this and the other _() thing is unrelated, but is need for this.
Because settings.rb now triggers loading all subclasses and _() does not work without settings

@durandom
Copy link
Member Author

durandom commented Sep 1, 2016

corresponding PR in the amazon repo ManageIQ/manageiq-providers-amazon#41

@durandom
Copy link
Member Author

durandom commented Sep 6, 2016

@Fryguy are you ok with the general direction of this?

@durandom durandom force-pushed the settings_for_providers branch from 3b42cc6 to 702a293 Compare September 6, 2016 07:15
@durandom
Copy link
Member Author

durandom commented Sep 6, 2016

commit 702a293 is also in #10964

@Fryguy
Copy link
Member

Fryguy commented Sep 8, 2016

@durandom Still thinking about this one...it feels kind of invasive at the various layers. One thing I like about @bdunne's approach is that it encapsulates all of that into a Vmdb::Plugins object. Then, I can see asking that class for provider_engines or something.

@bdunne
Copy link
Member

bdunne commented Sep 8, 2016

Also, this requires all plugins to subclass a provider. I have a couple use cases where I only want to provide an automate domain and a few non-provider models.

@durandom
Copy link
Member Author

durandom commented Sep 9, 2016

I'll wait until #11083 got merged and make use of Vmdb::Plugins then

@miq-bot
Copy link
Member

miq-bot commented Sep 14, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@chessbyte chessbyte changed the title [wip] Settings for providers [WIP] Settings for providers Sep 15, 2016
@durandom durandom force-pushed the settings_for_providers branch from 702a293 to 36c065f Compare October 27, 2016 14:16
@durandom durandom changed the title [WIP] Settings for providers Settings for providers Oct 27, 2016
@durandom
Copy link
Member Author

@miq-bot remove_label wip

@bdunne @Fryguy now it's much cleaner, thanks 🌹

regarding the order: I'm not sure if the plugins should be able to override core settings from manageiq (this is how its now) or the other way round.

@miq-bot miq-bot removed the wip label Oct 27, 2016
@durandom durandom force-pushed the settings_for_providers branch from 36c065f to e13fedb Compare November 9, 2016 10:45
@miq-bot
Copy link
Member

miq-bot commented Nov 9, 2016

Checked commit durandom@e13fedb with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
1 file checked, 0 offenses detected
Everything looks good. 👍

@durandom
Copy link
Member Author

durandom commented Nov 9, 2016

regarding the order: I'm not sure if the plugins should be able to override core settings from manageiq (this is how its now) or the other way round.

@Fryguy can you merge, if you are ok with this now?

@durandom
Copy link
Member Author

durandom commented Dec 6, 2016

@Fryguy ping

@Fryguy
Copy link
Member

Fryguy commented Dec 9, 2016

Looks really nice!

@Fryguy Fryguy merged commit 57d09fb into ManageIQ:master Dec 9, 2016
@Fryguy Fryguy added this to the Sprint 51 Ending Jan 2, 2017 milestone Dec 9, 2016
@durandom durandom deleted the settings_for_providers branch December 9, 2016 16:22
@durandom durandom mentioned this pull request Apr 21, 2017
38 tasks
@cben
Copy link
Contributor

cben commented May 1, 2017

Q: if we now add settings in say manageiq-providers-kubernetes/config/settings.yml and need the new settings backported, it will backport cleanly to fine but will need manual [EUWE] PRs, right?

Any chance this could be euwe/yes? I see #11083 was euwe/no.

Q: let's say you do add this support to euwe, would it help changes that become a hotfix onto say 5.7.1? I suppose no because this support would only go into later 5.7.z. Never mind then :)

@miq-bot miq-bot changed the title Settings for providers [WIP] Settings for providers May 1, 2017
@miq-bot miq-bot added wip and removed wip labels May 1, 2017
@bdunne
Copy link
Member

bdunne commented May 1, 2017

I don't see #11083 going back to euwe. It's too much risk at this point.

@durandom
Copy link
Member Author

durandom commented May 2, 2017

Q: if we now add settings in say manageiq-providers-kubernetes/config/settings.yml and need the new settings backported, it will backport cleanly to fine but will need manual [EUWE] PRs, right?

It could be, because the path is the same, but probably will not, because git/diff will not have enough context to apply the patch. So it probably be a manual PR.

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.

5 participants