Skip to content

Improve logging of environment variables #3543

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Improve logging of environment variables #3543

wants to merge 2 commits into from

Conversation

ssbarnea
Copy link
Member

@ssbarnea ssbarnea commented Jun 3, 2025

Improves logging of environment variables by sorting them by key and redacting
the values for the ones that are likely to contain secrets.

Fixes: #3542

  • ran the linter to address style issues (tox -e fix)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

pytest.param("ACCESS_TOKEN", True),
pytest.param("API_KEY", True),
pytest.param("DB_PASSWORD", True),
pytest.param("FOO", False),
Copy link
Member

Choose a reason for hiding this comment

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

Missing some of the ones included into rege x, let's test all of them, and we should document all of the patterns in our docs (users shouldn't need to read code to find it out).

This comment was marked as outdated.

This comment was marked as outdated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorted. Added tests for all of them and also included an extra logging message before the env vars are dumped that mentioned that some were redacted and why (keywords).

There is no need to update documentation website because there is no section reffering to tox logs. The only mention was on FAQ with a basic question about how to increase logging level. Also, due to the notice message in the log, we can consider the behavior self-documented.

Copy link
Member

@gaborbernat gaborbernat Jun 4, 2025

Choose a reason for hiding this comment

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

We should add that documentation section. I'm on holiday will review and merge when I'm back next week. I don't think we should have that notice message.

@ssbarnea ssbarnea force-pushed the fix/3542 branch 2 times, most recently from 302c9cf to 511fcac Compare June 4, 2025 11:56
@ssbarnea
Copy link
Member Author

ssbarnea commented Jun 4, 2025

@gaborbernat Any chance you could look at it again today? I think that I addressed the requests.

This patch is a blocker for improving the security of GHA pipelines as I would not want to disable log collection for tox. Thanks.

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.

logged environment variables are not sorted
2 participants