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

Update pylint and fix the new issues its spots #946

Merged
merged 5 commits into from
Jul 20, 2021

Conversation

paride
Copy link
Contributor

@paride paride commented Jul 14, 2021

Proposed Commit Message

Update pylint to v2.9.3 and fix the new issues its spots

In CI run against pylint 2.9.3 and fix occurrences of:
 - W0237 (arguments-renamed)
 - W0402 (deprecated-module)

The W0402 deprecated-module was about module `imp`:

    cloudinit/patcher.py:9: [W0402(deprecated-module), ]
        Uses of a deprecated module 'imp'

The imp module is deprecated and replaced by importlib, which according
to the documentation has no replacement for acquire_lock() and
release_lock(), which are the only reason why `imp` is imported.
    
I see nothing about the code using this lock that actually requires it.
Let's remove the locking code and the import altogether.

Dropping the locking makes patcher.patch() an empty wrapper around
_patch_logging(). Rename _patch_logging() to patch_logging() and
call it directly instead. Drop patch().

Additional Context

Spotted by our cloud-init-style-tip Jenkins job.

This is the job always running against the latest versions of the linters. The job allows to keep the code up-to-date with the new versions without breaking CI, which uses pinned versions instead.

Test Steps

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

Sorry, something went wrong.

@paride
Copy link
Contributor Author

paride commented Jul 14, 2021

This is marked as a WIP because it doesn't fix this yet:

cloudinit/patcher.py:9: [W0402(deprecated-module), ] Uses of a deprecated module 'imp'

as I'm not really sure on what to do. The imp module is deprecated and replaced by importlib, which according to the documentation has no replacement for acquire_lock() and release_lock(). I'm not sure if those calls are still needed, or if we should silence the warning for now.

Other projects are facing this, e.g. pypa/setuptools#479

@paride
Copy link
Contributor Author

paride commented Jul 15, 2021

CI failure is of course expected for now:

cloudinit/patcher.py:9: [W0402(deprecated-module), ] Uses of a deprecated module 'imp'

@TheRealFalcon
Copy link
Member

The imp module is deprecated and replaced by importlib, which according to the documentation has no replacement for acquire_lock() and release_lock(). I'm not sure if those calls are still needed, or if we should silence the warning for now.

I see nothing about the code using this lock that actually requires it. There's not multiple threads in play and we're not creating import hooks. I'm comfortable removing the locking code altogether.

@paride
Copy link
Contributor Author

paride commented Jul 19, 2021

Thanks! I agree it doesn't really seem needed. I'll update the branch and drop those.

Paride Legovini added 3 commits July 20, 2021 11:17
Specimen:

cloudinit/distros/rhel.py:81: [W0237(arguments-renamed),
Distro._write_hostname] Parameter 'filename' has been renamed to
'out_fn' in overridden 'Distro._write_hostname' method

cloudinit/distros/rhel.py:133: [W0237(arguments-renamed),
Distro.package_command] Parameter 'cmd' has been renamed to 'command' in
overridden 'Distro.package_command' method

cloudinit/distros/gentoo.py:152: [W0237(arguments-renamed),
Distro._write_hostname] Parameter 'hostname' has been renamed to
'your_hostname' in overridden 'Distro._write_hostname' method
The imp module is deprecated and replaced by importlib, which according
to the documentation has no replacement for acquire_lock() and
release_lock(), which are the only reason why `imp` is imported.

I see nothing about the code using this lock that actually requires it.
Let's remove the locking code and the import altogether.
@paride paride changed the title [WIP] Update pylint and fix the new issues its spots Update pylint and fix the new issues its spots Jul 20, 2021
@paride
Copy link
Contributor Author

paride commented Jul 20, 2021

Branch updated and proposed commit message updated. I removed the WIP as this is now ready for review.

Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Some minor requests inline

cloudinit/distros/__init__.py Outdated Show resolved Hide resolved
cloudinit/patcher.py Outdated Show resolved Hide resolved
Paride Legovini added 2 commits July 20, 2021 15:50
`out_fn` may cause confusion as `fn` may be interpreted as "function".
Also: remove the `noqa` annotation (qa passes).
@paride
Copy link
Contributor Author

paride commented Jul 20, 2021

PR updated and proposed commit message also updated.

Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Thanks!

@TheRealFalcon TheRealFalcon merged commit ec6afad into canonical:main Jul 20, 2021
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.

None yet

2 participants