Skip to content

Log environment variables sorted by key while redacting values of unsafe ones#3543

Merged
ssbarnea merged 1 commit intomainfrom
fix/3542
Jun 17, 2025
Merged

Log environment variables sorted by key while redacting values of unsafe ones#3543
ssbarnea merged 1 commit intomainfrom
fix/3542

Conversation

@ssbarnea
Copy link
Copy Markdown
Member

@ssbarnea ssbarnea commented Jun 3, 2025 •
edited
Loading

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

@ssbarnea ssbarnea marked this pull request as ready for review June 3, 2025 09:28
@ssbarnea ssbarnea requested a review from gaborbernat as a code owner June 3, 2025 09:28
@ssbarnea ssbarnea enabled auto-merge (squash) June 3, 2025 10:50
@ssbarnea ssbarnea mentioned this pull request Jun 3, 2025
Merged
gaborbernat
gaborbernat previously requested changes Jun 3, 2025
View reviewed changes
@ssbarnea ssbarnea force-pushed the fix/3542 branch 3 times, most recently from 511fcac to 7f508e0 Compare June 4, 2025 12:01
@ssbarnea ssbarnea requested a review from gaborbernat June 4, 2025 12:47
@ssbarnea
Copy link
Copy Markdown
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.

@ssbarnea ssbarnea changed the title Improve logging of environment variables Log environment variables sorted by key while redacting values of some unsafe ones Jun 10, 2025
@ssbarnea ssbarnea changed the title Log environment variables sorted by key while redacting values of some unsafe ones Log environment variables sorted by key while redacting values of unsafe ones Jun 10, 2025
@ssbarnea ssbarnea force-pushed the fix/3542 branch 2 times, most recently from ca01304 to 37a1156 Compare June 10, 2025 16:10
@ssbarnea ssbarnea dismissed gaborbernat’s stale review June 10, 2025 16:15

Please review again, I made the required changes, dropped the notice, added documentation entry, tested all keywords.

@ssbarnea
Copy link
Copy Markdown
Member Author

ssbarnea commented Jun 11, 2025

@gaborbernat Sorry to bother you again. Is there anything else I need to change?

gaborbernat
gaborbernat requested changes Jun 11, 2025
View reviewed changes
gaborbernat
gaborbernat approved these changes Jun 12, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

gaborbernat
gaborbernat previously requested changes Jun 12, 2025
View reviewed changes
@ssbarnea
Copy link
Copy Markdown
Member Author

ssbarnea commented Jun 12, 2025

@gaborbernat The py314 failure is unrelated and seems to be a bug in coveragepy. I will try to help @nedbat with a patch if I do not run out of time. That also made me realize that I need to fix few bugs in tox-uv in order to run coveragepy tests..., side effect raising tox-dev/tox-uv#210

@ssbarnea
Copy link
Copy Markdown
Member Author

ssbarnea commented Jun 12, 2025

@gaborbernat Any idea about what to do about py314 errors related to coverage?

CoverageWarning: Couldn't import C tracer: No module named 'coverage.tracer' (no-ctracer)

They started to fail on main last night, so unrelated to this patch. Should we attempt to capture/hide these warnings to avoid our test failures?

@nedbat
Copy link
Copy Markdown
Contributor

nedbat commented Jun 12, 2025

I think I will skip that warning on platforms where I don't ship a built wheel, though I'd be curious whether the installation of coverage on 3.14 shows that it tried to compile the extension or not?

@nedbat
Copy link
Copy Markdown
Contributor

nedbat commented Jun 13, 2025

Coverage 7.9.1 is released to quiet that warning on 3.14.

@ssbarnea ssbarnea requested a review from gaborbernat June 17, 2025 10:19
@ssbarnea ssbarnea dismissed gaborbernat’s stale review June 17, 2025 10:20

Sorted all comments, CI is reporting green now. Please review again so we can close it. Thanks

@ssbarnea ssbarnea merged commit 5077e20 into main Jun 17, 2025
28 checks passed
@ssbarnea ssbarnea deleted the fix/3542 branch June 17, 2025 15:14
@cidrblock
Copy link
Copy Markdown

cidrblock commented Jun 17, 2025

TY @gaborbernat

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

Reviewers

@gaborbernat gaborbernat gaborbernat approved these changes

Assignees

No one assigned

Labels

bot:chronographer:provided

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

logged environment variables are not sorted

4 participants

@ssbarnea @nedbat @cidrblock @gaborbernat