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

Adopt the custom_injectors callback #13

Merged
merged 2 commits into from
Nov 20, 2024
Merged

Adopt the custom_injectors callback #13

merged 2 commits into from
Nov 20, 2024

Conversation

chrismeyersfsu
Copy link
Member

No description provided.

webknjaz

This comment was marked as resolved.

Sign in to view

@webknjaz webknjaz mentioned this pull request Nov 19, 2024
Merged
@chrismeyersfsu

This comment was marked as resolved.

Sign in to view
@chrismeyersfsu chrismeyersfsu force-pushed the AAP-35749-move-inject-credential branch 7 times, most recently from 6995214 to 3922ce7 Compare November 20, 2024 11:36
@chrismeyersfsu
Copy link
Member Author

chrismeyersfsu commented Nov 20, 2024 •
edited
Loading

@webknjaz Unsure what to do about the doc failure.

@webknjaz
Copy link
Member

@webknjaz Unsure what to do about the doc failure.

@chrismeyersfsu one solution could be using this hack to produce artificial references to links somewhere in the source code: https://github.com/ansible/awx-plugins/pull/24/files#diff-85933aa74a2d66c3e4dcdf7a9ad8397f5a7971080d34ef1108296a7c6b69e7e3R209-R251.

webknjaz
webknjaz reviewed Nov 20, 2024
View reviewed changes
docs/conf.py Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

@chrismeyersfsu I'm trying something out. Hold on. We might be able to revert your commit with the ignore with what I'm trying to do.

@webknjaz webknjaz added the enhancement New feature or request label Nov 20, 2024
@webknjaz webknjaz force-pushed the AAP-35749-move-inject-credential branch from 11dbd7a to 41a4361 Compare November 20, 2024 17:26
webknjaz
webknjaz approved these changes Nov 20, 2024
View reviewed changes
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

@chrismeyersfsu everything works now. The branch could use a little cleanup, though.

@chrismeyersfsu chrismeyersfsu force-pushed the AAP-35749-move-inject-credential branch 2 times, most recently from 86e3b6b to 5b4a72f Compare November 20, 2024 18:35
@chrismeyersfsu
Copy link
Member Author

@webknjaz my cleaning up of the branch broke things or the fix didn't work.

@webknjaz webknjaz force-pushed the AAP-35749-move-inject-credential branch from 7f75fdd to 769791f Compare November 20, 2024 20:44
webknjaz
webknjaz reviewed Nov 20, 2024
View reviewed changes
src/awx_plugins/interfaces/_temporary_private_api.py Outdated
@@ -4,6 +4,13 @@
The hope is that it will be refactored into something more standardized.
"""

from collections.abc import Callable

from awx_plugins.interfaces._temporary_private_credential_api import ( # noqa: WPS436, pylint: disable=C0301
Copy link
Member

Choose a reason for hiding this comment

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

Urgh.. I missed this. Don't use cryptic numbers in pylint ignores. It's got proper human-readable names for rules...

Copy link
Member

Choose a reason for hiding this comment

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

Ahhhh... I see. That's line-too-long.

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, the root cause can be fixed because the linting violation actually reveals a different problem — an absolute import. This should be using relative imports instead. And then, there wouldn't be a problem to suppress in the first place. We shouldn't hide such problems, but work to understand why they happen.

Copy link
Member

Choose a reason for hiding this comment

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

If something needs to be ignored, there should be a code comment explaining why. Especially for uncommon flake8 rule codes. In case of pylint, they should be human-readable, which is often enough (but not always).

webknjaz
webknjaz reviewed Nov 20, 2024
View reviewed changes
src/awx_plugins/interfaces/_temporary_private_credential_api.py Outdated
@@ -0,0 +1,52 @@
# pylint: disable=fixme
Copy link
Member

Choose a reason for hiding this comment

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

Also, global pylint ignores like this should be used as the last resort. They should be scoped to a single line or fixed instead.

@chrismeyersfsu @webknjaz
Adopt the custom_injectors callback
55d26d4 
Co-Authored-By: Sviatoslav Sydorenko <[email protected]>
@webknjaz webknjaz force-pushed the AAP-35749-move-inject-credential branch from 769791f to ea13414 Compare November 20, 2024 20:55
@webknjaz webknjaz force-pushed the AAP-35749-move-inject-credential branch from ea13414 to bc0e0de Compare November 20, 2024 21:01
@webknjaz webknjaz merged commit 49aacb0 into ansible:devel Nov 20, 2024
31 of 32 checks passed
@webknjaz
Copy link
Member

@chrismeyersfsu post-merge instructions:

  1. Click Run workflow @ https://github.com/ansible/awx_plugins.interfaces/actions/workflows/ci-cd.yml, entering 0.0.1a2 precisely in the version field. Don't enter anything else and submit the form.
  2. Wait until the newly created workflow run completes tests and pauses, then approve releasing on the workflow run summary page.
  3. Wait until the workflow run completes fully.
  4. Proceed with the instructions posted @ Move injectors to ManagedCredentialType awx-plugins#57 (comment).

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

@webknjaz webknjaz webknjaz approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chrismeyersfsu @webknjaz