-
Notifications
You must be signed in to change notification settings - Fork 87
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
Fixes #12751 - --plugin-name renamed to --provider #187
Conversation
@mbacovsky @tstrachota I think those changes are good, sticking with 1 name convention to reduce confusion (provider vs plugin). |
In the second commit I've added some thoughts on refactoring. I changed Providers to be used as an instance, added description and renamed method The foreman provider would look like this: require 'hammer_cli'
module HammerCLIForeman
class TaxonomiesDefaults < HammerCLI::BaseDefaultsProvider
def initialize
@provider_name = 'taxonomies'
@supported_defaults = [:organization_id, :location_id]
@description = _('Use the default organization and/or location from the server')
end
def get_defaults(param)
param = "default_organization" if param == :organization_id
param = "default_location" if param == :location_id
user = HammerCLIForeman.foreman_resource(:users).action(:index).call(:search =>"login="+HammerCLIForeman.credentials.username)
val = nil
if user
val = user["results"].first[param] if user["results"]
val = val["id"] if val.is_a?(Hash) && param.include?("default")
end
val
end
end
HammerCLI.defaults.register_provider(TaxonomiesDefaults.new())
end @alongoldboim, please take a look if this is something we want in. |
@mbacovsky it looks good to me and i like that we are using instance variables instead of methods so BaseDefaultsProvider is clearer, I still think keeping the provider name as foreman is more user friendly but i have been mistaken before... :) |
|
- fixed minor formatting issues - fixed someplugin/provider inconsistencies - updated tests - added description - Provider to be used as a instance
68faea4
to
abb67f8
Compare
Squashed |
I forgot to mention I like the provider description too. Good idea! |
Fixes #12751 - --plugin-name renamed to --provider
BTW changing the tests to work with regex is a lot better then the previous version 👍 |
defaults add
I propose removal of the provider default name generated from the plugin name. The reason is that there is no relation between the provider name and the plugin name. I'd prefer one plugin to offer rather more simplier defaults providers then one complex provider. E.g. foreman plugin could implement
taxonomies
provider providing orgs and locations.It would be nice to add description to the provider and print it in
defaults providers
so that the user knows what is it good for. What do you think? @alongoldboim, @tstrachota