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

Make base_serial optional in LSHW parsing #517

Merged
merged 2 commits into from
Mar 13, 2017

Conversation

byxorna
Copy link
Contributor

@byxorna byxorna commented Mar 8, 2017

This fixes a bug where the BASE_SERIAL attribute isnt correctly filtered when reconstructing base LSHW attributes for display. This caused the base_serial attribute to appear multiple times (one for each time it was inducted!), even though this attribute shouldn't have been displayed here at all (it should show up on the LSHW show page, not generic attributes).

Ive also spiced up the asset overview page with some tooltips when the Label for an attribute doesnt match the Name, to make it easier to understand where the attribute comes from. Additionally, I've added some docs links to describe the classification, type, status, etc of an asset.

Fixes #477
cc @roymarantz @defect @qx-xp @michaeljs1990 @gtorre

screen shot 2017-03-08 at 1 33 48 pm

@michaeljs1990
Copy link
Contributor

This is awesome and always bugged me since I have some assets with this showing up 3 times right now.

Copy link
Contributor

@roymarantz roymarantz left a comment

Choose a reason for hiding this comment

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

👍
although it would be better if some unit tests were added/fixed.
Also please squash the commit history to clean it up a bit.

drop duplicate BASE_SERIAL attrs

add some docs and tooltips to attributes in show_overview

fix some tooltips
@byxorna
Copy link
Contributor Author

byxorna commented Mar 8, 2017

@roymarantz ack! ive rebased and squashed down.
Unfortunately this is very much a view issue, for which we dont have a framework for testing. I would be hesitant to take on adding a whole approach to validating views in this diff, as it seems like a massive amount of work at first blush

@roymarantz
Copy link
Contributor

good enough. thanks

@defect
Copy link
Contributor

defect commented Mar 11, 2017

Hot 🔥 LGTM

@byxorna byxorna merged commit 347b156 into tumblr:master Mar 13, 2017
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.

Base Serial How does your computer identify itself?
4 participants