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

Remove the Hardware#m_bios method #20258

Closed
wants to merge 1 commit into from
Closed

Remove the Hardware#m_bios method #20258

wants to merge 1 commit into from

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Jun 5, 2020

This is part of the ManageIQ/manageiq-gems-pending#231 effort to remove miq-uuid as a dependency anywhere except vmware.

Since this method doesn't appear to be used, let's remove it.

Cross repo tests: ManageIQ/manageiq-cross_repo-tests#140

@miq-bot
Copy link
Member

miq-bot commented Jun 5, 2020

Checked commit https://github.com/djberg96/manageiq/commit/81b291da0c846f9c1b299fc08408af052f08833d with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@@ -210,14 +210,6 @@ def m_memory(_parent, xmlNode, _deletes)
self.memory_mb = xmlNode.attributes["memsize"]
end

def m_bios(_parent, xmlNode, _deletes)
Copy link
Member

Choose a reason for hiding this comment

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

@roliveri @jerryk55 I assume this was used for smartstate scans, does this look familiar to you? Was it previously used but not anymore?

Copy link
Member

Choose a reason for hiding this comment

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

@agrare if it was it was from before my time. I don't ever remember seeing it used.

Copy link
Member

Choose a reason for hiding this comment

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

I don't recall. Are the similar routines (like m_vm) currently used by SSA?

Copy link
Member

Choose a reason for hiding this comment

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

So I think the answer to "is this used anymore" is a definite yes. There are a number of m_ methods that appear to be called by smartstate (#20258 (comment))

@Fryguy
Copy link
Member

Fryguy commented Jun 9, 2020

parent.hardware.send("m_#{e.name}", parent, e, deletes) if parent.hardware.respond_to?("m_#{e.name}")

Found via ag "\bm_#", but also can be found with org:ManageIQ m_#

@djberg96
Copy link
Contributor Author

@Fryguy well what would you like me to do then?

@Fryguy
Copy link
Member

Fryguy commented Jun 10, 2020

Considering the method is used, I think you have to keep it. Unless you can show that the results from a smart state scan won't have the bios value anymore

@agrare
Copy link
Member

agrare commented Jun 10, 2020

@djberg96 Since you're trying to get rid of MiqUUID.clean_guid, and that is vmware specific, I think we should just drop the MiqUUID.clean_guid from core Hardware and clean the guid in vmware specific smartstate code that pulls the value

@chessbyte
Copy link
Member

Closing because we cannot remove the m_bios method

@chessbyte chessbyte closed this Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants