-
Notifications
You must be signed in to change notification settings - Fork 900
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
Remove reference to job.guid from VmOrTemplate #15009
Remove reference to job.guid from VmOrTemplate #15009
Conversation
…es are instances of MiqServer and does not depend on 'job'. proxy can not be instance of Host and we do not need to check for registred hosts
…s of SmartState proxy through registered hosts: proxy can not be instance of Host - only MiqServer $rspec: no need for tests SmartState proxy through registered hosts
c8a7238
to
8758d7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me 👍 I would like to get another set of eyes on it, though, before merging. @jrafanie, can you please review? Thanks!
|
||
log_all_proxies(all_proxy_list, msg) if proxies.empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can log_proxies
be removed down below? Are there other callers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up PR will remove not used VmOrTemplate#log_proxies and replace call to it to log_all_proxies for redhat provider
Lol, now I see it, sorry
if job && !job.kind_of?(ActiveRecord::Base) | ||
jobid = job | ||
job = Job.find_by(:guid => jobid) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, we do all that work with the job just for logging... Nice find. We don't need it.
elsif proxies.empty? | ||
"Provide credentials for this VM's Host to perform SmartState Analysis" | ||
else | ||
'Perform SmartState Analysis on this VM' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yrudman I'm good with this whole PR but this code is not sufficiently covered by tests, did you run SSA for the 3 conditionals you're handling here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrafanie I've added missing test
a266405
to
ee3d7e0
Compare
Checked commits yrudman/manageiq@17e8652~...ee3d7e0 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
There is no need to include
job.guid
into error message when proxy for smart state analysis not available - proxy is instance ofMiqServer
and does not depend on job instance.This PR:
VmOrTemplate#log_all_proxies' which does not accept
job`job
Host
been proxy for SmartState AnalysisFollow-up PR will remove not used
VmOrTemplate#log_proxies
and replace call to it tolog_all_proxies
forredhat
provider@miq-bot add-label technical debt