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

pipx: add testcase w/ env vars PIPX_xxxx #5845

Merged

felixfontein merged 7 commits into ansible-collections:main from russoz-ansible:5813-pipx-env

Jan 18, 2023

Collaborator

russoz commented Jan 16, 2023 •

edited

Loading

SUMMARY

Adds a testcase to demonstrate how the pipx module can be used with the environment variables PIPX_*, specifically for site-wide installation.

Fixes #5813

ISSUE TYPE

Docs Pull Request

COMPONENT NAME

pipx
pipx_info

russoz added 2 commits

January 16, 2023 20:46


          pipx: add testcase w/ env vars PIPX_xxxx

5d31701


          add note to the docs about env vars

cde6d7b

ansibullbot added docs integration language module packaging plugins tests labels

russoz mentioned this pull request

pipx module should expose control of install directories #5813

Closed

1 task

ansibullbot added the has_issue label

github-actions bot commented Jan 16, 2023 •

edited

Loading

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.


          add note to the docs about env vars

9350c07

gdevenyi commented Jan 16, 2023

This is okay, but it doesn't actually solve the discoverability problem that I didn't know that environment: is an option available to me. Perhaps it could like the appropriate docs?

Collaborator Author

russoz commented Jan 16, 2023

I actually added that note to the docs as well.

gdevenyi commented Jan 16, 2023

I see the addition, however that does not tell me as a user that environment: exists in ansible and I can use it to control those features. I didn't know it did.

felixfontein approved these changes

View reviewed changes

felixfontein added check-before-release backport-5 labels

russoz commented

View reviewed changes

plugins/modules/pipx.py Outdated Show resolved Hide resolved

russoz commented

View reviewed changes

plugins/modules/pipx_info.py Outdated Show resolved Hide resolved


          Apply suggestions from code review

1a890e5

Collaborator Author

russoz commented Jan 18, 2023

I see the addition, however that does not tell me as a user that environment: exists in ansible and I can use it to control those features. I didn't know it did.

I added a mention to that. It does feel like repeating Ansible documentation in the modules though.

This comment was marked as outdated.

Sign in to view

ansibullbot added ci_verified needs_revision labels

felixfontein reviewed

View reviewed changes

plugins/modules/pipx.py Outdated Show resolved Hide resolved

plugins/modules/pipx_info.py Outdated Show resolved Hide resolved

Collaborator

felixfontein commented Jan 18, 2023

It does feel like repeating Ansible documentation in the modules though.

That is indeed something we should avoid. I'm still not convinced it is a good idea here.


          Update plugins/modules/pipx.py

2aab2c1

Co-authored-by: Felix Fontein <felix@fontein.de>

russoz and others added 2 commits

January 18, 2023 21:11


          Update plugins/modules/pipx_info.py

1d9ca60

Co-authored-by: Felix Fontein <felix@fontein.de>


          break long lines into smaller ones

98688f7

ansibullbot removed ci_verified needs_revision labels

gdevenyi commented Jan 18, 2023

That is indeed something we should avoid. I'm still not convinced it is a good idea here.

If the documentation was written as per the prior version of this PR, I would've opened up an issue like #5813 instead asking "how do I do this, the documentation doesn't say"

Collaborator Author

russoz commented Jan 18, 2023

True, and if the Ansible documentation was consulted first it is likely there would be no PR at all. If we follow that logic we should add this type of note to all modules that are affected by env variables, and I risk a guess that there are a couple of hundred in that situation. Then if we factor in all other Ansible features used by modules, this could potentially escalate quickly to a massive amount of "docs" that just point to existing stuff.

felixfontein approved these changes

View reviewed changes

felixfontein merged commit 1430ed0 into ansible-collections:main

patchback bot commented Jan 18, 2023 •

edited

Loading

Backport to stable-5: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-5/1430ed000c704fb03f0ad65d25285c0921f012a0/pr-5845

Backported as #5858

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

felixfontein removed the check-before-release label

patchback bot pushed a commit that referenced this pull request


          pipx: add testcase w/ env vars PIPX_xxxx (#5845)

* pipx: add testcase w/ env vars PIPX_xxxx

* add note to the docs about env vars

* add note to the docs about env vars

* Apply suggestions from code review

* Update plugins/modules/pipx.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/modules/pipx_info.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* break long lines into smaller ones

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 1430ed0)

patchback bot mentioned this pull request

[PR #5845/1430ed00 backport][stable-5] pipx: add testcase w/ env vars PIPX_xxxx #5858

Merged

patchback bot commented Jan 18, 2023 •

edited

Loading

Backport to stable-6: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-6/1430ed000c704fb03f0ad65d25285c0921f012a0/pr-5845

Backported as #5859

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

patchback bot pushed a commit that referenced this pull request


          pipx: add testcase w/ env vars PIPX_xxxx (#5845)

* pipx: add testcase w/ env vars PIPX_xxxx

* add note to the docs about env vars

* add note to the docs about env vars

* Apply suggestions from code review

* Update plugins/modules/pipx.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/modules/pipx_info.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* break long lines into smaller ones

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 1430ed0)

patchback bot mentioned this pull request

[PR #5845/1430ed00 backport][stable-6] pipx: add testcase w/ env vars PIPX_xxxx #5859

Merged

Collaborator

felixfontein commented Jan 18, 2023

@russoz thanks for your contribution!
@gdevenyi thanks for commenting on this!

russoz deleted the 5813-pipx-env branch

January 18, 2023 20:37

felixfontein pushed a commit that referenced this pull request


          [PR #5845/1430ed00 backport][stable-5] pipx: add testcase w/ env vars…

b66df69

… PIPX_xxxx (#5858)

pipx: add testcase w/ env vars PIPX_xxxx (#5845)

* pipx: add testcase w/ env vars PIPX_xxxx

* add note to the docs about env vars

* add note to the docs about env vars

* Apply suggestions from code review

* Update plugins/modules/pipx.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/modules/pipx_info.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* break long lines into smaller ones

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 1430ed0)

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

felixfontein pushed a commit that referenced this pull request


          [PR #5845/1430ed00 backport][stable-6] pipx: add testcase w/ env vars…

1ade62c

… PIPX_xxxx (#5859)

pipx: add testcase w/ env vars PIPX_xxxx (#5845)

* pipx: add testcase w/ env vars PIPX_xxxx

* add note to the docs about env vars

* add note to the docs about env vars

* Apply suggestions from code review

* Update plugins/modules/pipx.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/modules/pipx_info.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* break long lines into smaller ones

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 1430ed0)

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

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

Labels

docs has_issue integration language module packaging plugins tests