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

If product_name is set to OEM message read board name instead #50396

Open
wants to merge 3 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@mstarikov
Copy link

mstarikov commented Dec 30, 2018

SUMMARY

Some desktop motherboards(like gigabyte) may not have product field set. Instead we can use "Product Name" from the "Base Board Information" section of dmi to provide meaningful value.
e.g.

System Information
Manufacturer: Gigabyte Technology Co., Ltd.
Product Name: To be filled by O.E.M.
...
Base Board Information
Manufacturer: Gigabyte Technology Co., Ltd.
Product Name: H61MA-D2V

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

module_utils/facts/hardware/linux.py

ADDITIONAL INFORMATION

very specific string to check for "To be filled by O.E.M." don't know if there are any other values that might come up for unset product_name field.

Not sure if there is any try/except required here since presence of dmi keys confirmed above and get_file_conent handles any empty/missing files.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 30, 2018

@mstarikov: Greetings! Thanks for taking the time to open this pullrequest. In order for the community to handle your pullrequest effectively, we need a bit more information.

Here are the items we could not find in your description:

  • issue type

Please set the description of this pullrequest with this template:
https://raw.githubusercontent.com/ansible/ansible/devel/.github/PULL_REQUEST_TEMPLATE.md

click here for bot help

@sivel

This comment has been minimized.

Copy link
Member

sivel commented Jan 8, 2019

Thank you very much for this submission. We do not believe that this should be special cases like this. Instead we recommend one of the following:

  1. Create a local fact for that machine
  2. Have your playbook handle this situation
  3. Change this PR to add a new fact for board, and do not overwrite product and leaving product name as is.

needs_revision

@mstarikov

This comment has been minimized.

Copy link

mstarikov commented Jan 9, 2019

Hi Matt,

Thanks for looking into that.

We do not believe that this should be special cases like this.

Could I ask you to expand on what is the motivation behind that decision?

The "if statement" triggered on very particular string so wouldn't affect any other functionality and I'm sure this change wouldn't add much(if at all) to the execution time.

I understand that currently ansible is used primarily on servers and VMs, but if it to become a default standard configuration tool(I certainly hope so) and reach desktops that would affect pretty much every off-the-shelf motherboard.

@bcoca

This comment has been minimized.

Copy link
Member

bcoca commented Jan 10, 2019

@mstarikov we already took that into account and would still prefer a new fact vs overwriting the existing fact with a 2ndary value. If looking at the data directly the users would still have to go to the 2nd source and the change we ask for would reflect that.

@mstarikov

This comment has been minimized.

Copy link

mstarikov commented Jan 14, 2019

Thanks for the reply @bcoca. Yes, of course that makes sense. I'm not sure what I was thinking with the first commit - silently replacing data source is the kind of dodgy practice that would always get you in trouble. Anywho, Just pushed commit by adding board_name in both /sys and dmidecode. Please advise.

@ansibot ansibot removed the stale_ci label Jan 14, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jan 14, 2019

@mstarikov this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@mstarikov mstarikov force-pushed the mstarikov:devel branch from 0713686 to db46073 Jan 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment