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

New method for getting the instance without domain prefix. #18722

Merged
merged 2 commits into from May 21, 2019

Conversation

@pkomanek
Copy link
Contributor

pkomanek commented May 3, 2019

Description

Modifying the MiqAeInstance.get_homonymic_across_domains method to get the MiqAeInstance by fqname without domain prefix if the :prefix parameter is false.

Example:
MiqAeInstance.get_homonymic_across_domains(user, fqname_without_domain_prefix, :prefix => false)
-> result: Returns an array of matching results sorted by domain priority.

Links

Copy link
Member

skateman left a comment

The Seal of Approval
Works as expected, at least for my stuff

let(:c2) { FactoryBot.create(:miq_ae_class, :namespace_id => n2.id, :name => "class") }

before do
@i1 = FactoryBot.create(:miq_ae_instance, :class_id => c1.id, :name => "instance")

This comment has been minimized.

Copy link
@skateman

skateman May 9, 2019

Member

yes, you shouldn't do that, use let! instead

@pkomanek pkomanek changed the title [WIP] New method for getting the instance without domain prefix. New method for getting the instance without domain prefix. May 9, 2019
@miq-bot miq-bot removed the wip label May 9, 2019
Copy link
Member

tinaafitz left a comment

@pkomanek Looks good.

@@ -136,6 +136,12 @@ def self.get_homonymic_across_domains(user, fqname, enabled = nil)
MiqAeDatastore.get_homonymic_across_domains(user, ::MiqAeInstance, fqname, enabled)
end

def self.get_homonymic_across_domains_noprefix(user, path, enabled = nil)
return [] if path.blank?

This comment has been minimized.

Copy link
@lfu

lfu May 14, 2019

Member

What if path is a fully qualified path name?

This comment has been minimized.

Copy link
@pkomanek

pkomanek May 15, 2019

Author Contributor

It just parse the path => 'domain/namespace1/namespace2/class/instance' to { namespace => 'domain/namespace1/namespace2', klass => 'class', instance => 'instance'} .... and in that case it mostly returns blank array ([]), because there is no such a namespace. It could return some object only if there will be another miq_ae_instance with fqname equal to 'some_domain/fqname_from_method_call'. This is the only case I have in my mind.

end

context 'with disabled domains' do
let(:d1) { FactoryBot.create(:miq_ae_domain_disabled, :name => 'dom1', :priority => 3) }

This comment has been minimized.

Copy link
@lfu

lfu May 14, 2019

Member

Rather than creating a disabled domain, can we modify d1 to be disabled?

before do
d1.enabled = false
d1.save!
end

This comment has been minimized.

Copy link
@lfu

lfu May 15, 2019

Member
before { d1.update_attributes(:enabled => false) }
@lfu
lfu approved these changes May 15, 2019
@@ -136,6 +136,12 @@ def self.get_homonymic_across_domains(user, fqname, enabled = nil)
MiqAeDatastore.get_homonymic_across_domains(user, ::MiqAeInstance, fqname, enabled)
end

def self.get_homonymic_across_domains_noprefix(user, path, enabled = nil)

This comment has been minimized.

Copy link
@gmcculloug

gmcculloug May 17, 2019

Member

@mkanoor Do you think this should be exposed as a separate method from get_homonymic_across_domains to the caller or does it make sense to have a single method for the caller and have it take a parameter?

I was thinking we retain the original method name as the exposed method to callers and use a keyword argument for prefix, where the default is true. (get_homonymic_across_domains_noprefix would become a private method.)

Something like:

def self.get_homonymic_across_domains(user, fqname, enabled = nil, prefix: true)
  return get_homonymic_across_domains_noprefix(user, path, enabled) unless prefix

  MiqAeDatastore.get_homonymic_across_domains(user, ::MiqAeInstance, fqname, enabled)
end

def self.get_homonymic_across_domains_noprefix(user, path, enabled = nil)
...

Since prefix is a keyword argument the caller is not required to pass the enabled parameter if they want set prefix. Meaning, someone could call:

get_homonymic_across_domains(user, fqname, :prefix => false)

and the enabled parameter would still default to nil as expected.

This comment has been minimized.

Copy link
@mkanoor

mkanoor May 20, 2019

Contributor

@gmcculloug Your suggestion would lead to a better implementation

@pkomanek

This comment has been minimized.

Copy link
Contributor Author

pkomanek commented May 21, 2019

It looks like a better way. Thanks @gmcculloug @mkanoor

n_('Automate Instance', 'Automate Instances', number)
end

def self.get_homonymic_across_domains(user, fqname, enabled = true, prefix: true)

This comment has been minimized.

Copy link
@gmcculloug

gmcculloug May 21, 2019

Member

@pkomanek Why did the enabled default change from nil to true. I would not expect that value to change.

This comment has been minimized.

Copy link
@pkomanek

pkomanek May 21, 2019

Author Contributor

@gmcculloug my fault. Fixed.

@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented May 21, 2019

Checked commits pkomanek/manageiq@08b0ec2~...8ddc4f4 with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐️

@gmcculloug gmcculloug merged commit a6c07c4 into ManageIQ:master May 21, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.003%) to 73.743%
Details
@pkomanek pkomanek deleted the pkomanek:bz1553846_get_instance_by_fqname_without_domain_prefix_method branch May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.