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.

Sign up for GitHub

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

Jump to bottom

Fix the import path for pulp-hashlib #4007

Merged
merged 1 commit into from Jul 12, 2023
Merged

Fix the import path for pulp-hashlib #4007

merged 1 commit into from Jul 12, 2023

Conversation

lubosmj
Copy link
Member

@lubosmj lubosmj commented Jul 10, 2023 •
edited

In this commit, there were also removed redundant #noqa directives that
were used in the plugin API. The flake8 configuration had to be adjusted
accordingly to ignore the import errors.

closes #4006

mdellweg
mdellweg previously approved these changes Jul 10, 2023
View reviewed changes
Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

Wow, how could this have gone unnoticed?

Can you remove the #noqa for something more specific?

@lubosmj
Copy link
Member Author

lubosmj commented Jul 10, 2023

After some discussion, I am removing all # noqa clauses and updating the flak8 configuration to ignore the import errors in the plugin API.

@lubosmj lubosmj marked this pull request as draft July 10, 2023 15:18
@lubosmj lubosmj force-pushed the fix-import-plugin-pulp-hashlib branch 3 times, most recently from c0abf73 to 7cebc1d Compare July 10, 2023 15:45
@lubosmj
Fix the import path for pulp-hashlib
7e85e61 
In this commit, there were also removed redundant #noqa directives that
were used in the plugin API. The flake8 configuration had to be adjusted
accordingly to ignore the import errors.

closes pulp#4006
@lubosmj lubosmj force-pushed the fix-import-plugin-pulp-hashlib branch from 7cebc1d to 7e85e61 Compare July 10, 2023 15:51
@lubosmj lubosmj marked this pull request as ready for review July 10, 2023 15:52
@lubosmj
Copy link
Member Author

lubosmj commented Jul 10, 2023

I made a trade-off between ignored __init__.py files and plugin API files. Listing particular files seems to be cumbersome in the flake8 configuration. And, some of the files in the plugin API are worth inspecting the imported modules (e.g.,

pulpcore/pulpcore/plugin/actions.py

Line 17 in e152b3d

class ModifyRepositoryActionMixin:
)

@lubosmj lubosmj dismissed mdellweg’s stale review July 10, 2023 15:59

The #noqa were removed only from init.py files. Other files now contain the specific error.

mdellweg
mdellweg reviewed Jul 10, 2023
View reviewed changes
Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

Have you considered doing the __all__ = [...]? I think that is what is actually supposed to be done in python.

.flake8
@@ -7,6 +7,8 @@
[flake8]
exclude = ./docs/*,*/migrations/*,./pulpcore/app/protobuf/*

per-file-ignores = */__init__.py: F401
Copy link
Member

Choose a reason for hiding this comment

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

You may need to make this change in the plugin-template. It has a DO NOT EDIT remark.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, yes. 😿

Copy link
Member Author

Choose a reason for hiding this comment

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

pulp/plugin_template#764

@lubosmj
Copy link
Member Author

lubosmj commented Jul 11, 2023

The purpose of __all__ is to specify which modules should be exported when doing from XYZ import *. Personally, I do not see any reason for declaring all the modules once again just for the sake of the flake8 linter. I am against it.

gerrod3
gerrod3 reviewed Jul 12, 2023
View reviewed changes
pulpcore/app/models/__init__.py
Comment on lines -70 to -76
# This can lead to circular imports with a custom user model depending on this very module
# Moved to plugin/models/role.py to avoid the circular import.
# from .role import ( # noqa
# GroupRole,
# Role,
# UserRole,
# )
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this comment really be deleted?

Copy link
Member Author

@lubosmj lubosmj Jul 12, 2023 •
edited

Choose a reason for hiding this comment

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

Yes. It is redundant. On the other hand, could we bring back the imports in the next breaking release because of cd342f3?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe.

@lubosmj lubosmj merged commit 7f1e3ac into pulp:main Jul 12, 2023
15 checks passed
@patchback
Copy link

patchback bot commented Jul 12, 2023 •
edited

Backport to 3.28: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.28/7f1e3ac21d3f7fa9e63c06e6e6e9b512de661567/pr-4007

Backported as #4020

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@patchback patchback bot mentioned this pull request Jul 12, 2023
Closed
@patchback
Copy link

patchback bot commented Jul 13, 2023 •
edited

Backport to 3.28: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.28/7f1e3ac21d3f7fa9e63c06e6e6e9b512de661567/pr-4007

Backported as #4034

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@patchback patchback bot mentioned this pull request Jul 13, 2023
Closed
@patchback
Copy link

patchback bot commented Jul 14, 2023 •
edited

Backport to 3.28: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.28/7f1e3ac21d3f7fa9e63c06e6e6e9b512de661567/pr-4007

Backported as #4048

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@patchback patchback bot mentioned this pull request Jul 14, 2023
Merged
@patchback
Copy link

patchback bot commented Jul 14, 2023 •
edited

Backport to 3.29: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.29/7f1e3ac21d3f7fa9e63c06e6e6e9b512de661567/pr-4007

Backported as #4049

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@patchback patchback bot mentioned this pull request Jul 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggainey ggainey ggainey approved these changes

@mdellweg mdellweg mdellweg approved these changes

@gerrod3 gerrod3 gerrod3 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot import pulp_hashlib from the plugin API
4 participants
@lubosmj @ggainey @mdellweg @gerrod3