Skip to content

core.py: ignore wrong product_name files#53326

Merged
dwoz merged 2 commits into
saltstack:masterfrom
aplanas:fix_grains
Apr 20, 2020
Merged

core.py: ignore wrong product_name files#53326
dwoz merged 2 commits into
saltstack:masterfrom
aplanas:fix_grains

Conversation

@aplanas
Copy link
Copy Markdown
Contributor

@aplanas aplanas commented May 31, 2019

Some firmwares (like some NUC machines), do not provide valid
/sys/class/dmi/id/product_name strings. In those cases an
UnicodeDecodeError exception happens.

This patch ignore this kind of issue during the grains creation.

@aplanas
Copy link
Copy Markdown
Contributor Author

aplanas commented Jun 3, 2019

Added tests and remove duplicate dead code for AIX (maybe comes from a wrong rebase in the merge-forward mega PR : (( )

@Ch3LL Ch3LL requested a review from dwoz June 6, 2019 19:44
Ch3LL
Ch3LL previously approved these changes Jun 6, 2019
Copy link
Copy Markdown
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

can you take a look at the lint failures and clean those up?

@aplanas aplanas force-pushed the fix_grains branch 3 times, most recently from 1a67789 to e3351f4 Compare June 7, 2019 09:18
@aplanas
Copy link
Copy Markdown
Contributor Author

aplanas commented Jun 7, 2019

@Ch3LL indeed, done

Comment thread salt/grains/core.py Outdated
@Ch3LL
Copy link
Copy Markdown
Contributor

Ch3LL commented Jun 25, 2019

ping @dwoz bump can you review?

@aplanas
Copy link
Copy Markdown
Contributor Author

aplanas commented Jul 22, 2019

The ci/lint complain is a bit weird, as complains for some code that this PR do not touch:

14:06:56  ************* Module salt.grains.core
14:06:56  salt/grains/core.py:51: [W1505(deprecated-method), linux_distribution] Using deprecated method _deprecated_linux_distribution()

@aplanas aplanas requested a review from a team as a code owner August 2, 2019 13:59
@Ch3LL
Copy link
Copy Markdown
Contributor

Ch3LL commented Aug 2, 2019

ping @s0undt3ch any ideas why we aren't only checking the lines changed here? I checked the settings in noxfile and it looks like we are just running pylint against the files changed, not the lines changed.

@waynew
Copy link
Copy Markdown
Contributor

waynew commented Aug 2, 2019

@Ch3LL IIUC we've always been doing that, since before nox. I know I've had to fix code in unchanged parts of files that I've touched. I tried running pylint against our whole codebase once and it came up with hundreds, if not thousands of errors and warnings.

I'm not sure if a) we should keep it as-is, b) it's possible/we should change it to ignore untouched lines, or c) we should make a massive lint fix.

My initial impression is that a) is a fine approach.... but this PR is definitely not the place for a discussion about that :)

@s0undt3ch
Copy link
Copy Markdown
Contributor

PyLint is not capable of checking changed lines.
I'm on my phone so I can't see right away, but I bet this is against develop.
If that's the cases, I started to clean the hundreds of lint errors(because I had to upgrade PyLint due to switching to py3 only, and also because I removed some of the ignores), but that work is not yet complete.
So, expect lint issues, and please fix if so inclided, on PRs against develop.
I'll try to get that work completed once I'm back from PTO.

@s0undt3ch
Copy link
Copy Markdown
Contributor

By fix, I mean not all of the lint issues, just the ones triggered on PRs on the changed files.

@Ch3LL
Copy link
Copy Markdown
Contributor

Ch3LL commented Aug 9, 2019

thanks for the clarifications, i was mistaken and thought we were previously doing this.

I put a PR in here: #54163 to ignore this pylint warning

waynew
waynew previously approved these changes Aug 28, 2019
Comment thread salt/grains/core.py Outdated
@aplanas aplanas changed the base branch from develop to master October 14, 2019 11:53
@aplanas
Copy link
Copy Markdown
Contributor Author

aplanas commented Oct 14, 2019

Rebased into master

Ch3LL
Ch3LL previously approved these changes Apr 15, 2020
@Ch3LL
Copy link
Copy Markdown
Contributor

Ch3LL commented Apr 15, 2020

ping @waynew can you re-review here?

aplanas and others added 2 commits April 17, 2020 16:48
Some firmwares (like some NUC machines), do not provide valid
/sys/class/dmi/id/product_name strings. In those cases an
UnicodeDecodeError exception happens.

This patch ignore this kind of issue during the grains creation.
@dwoz dwoz merged commit e7e9fd6 into saltstack:master Apr 20, 2020
@aplanas aplanas deleted the fix_grains branch April 20, 2020 08:40
@sagetherage sagetherage added the ZRelease-Sodium retired label label May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants