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.

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

Don't clear environment variables #414

Merged
merged 4 commits into from
Apr 30, 2024
Merged

Conversation

dbnicholson
Copy link
Contributor

See the first commit for discussion, but basically it can't be done correctly like this and it was causing the GIT_AUTHOR_* environment variables to be unset when making commits. A couple tests are added to make sure that doesn't happen again.

Fixes: #413

Copy link
Contributor

@wjt wjt left a comment

Choose a reason for hiding this comment

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

LGTM – presumably if you rebase the test failures will disappear since your branch at #415 was merged?

@dbnicholson
Copy link
Contributor Author

LGTM – presumably if you rebase the test failures will disappear since your branch at #415 was merged?

It should. Rebased now.

@dbnicholson
Copy link
Contributor Author

LGTM – presumably if you rebase the test failures will disappear since your branch at #415 was merged?

It should. Rebased now.

And yet flake8 has done its job and found and unused import.

The logic of it makes sense - prevent programs from accessing sensitive
environment variables. However, we have no idea whether the programs
being executed depend on those environment variables. It would be
entirely understandable if a tool to inspect a remote repository
required authentication, for example. The broad filtering being done
also makes it likely that an insensitive environment variable was
removed. There's really no way to win that game.

This also fixes an issue where GIT_AUTHOR_NAME and GIT_AUTHOR_EMAIL were
being filtered out of the commit command. This appears to be a bug in
python since the altered environment somehow escaped past its usage and
only via subprocess. In any case, removing the filtering fixes the
problem.

Fixes: flathub-infra#413
In order to test the data added to git commits, clear the associated
environment variables so that the test fully manages inputs.
Test that the git commit was made with the correct values in the
metadata. Primarily this is useful to make sure that commit authorship
is being done correctly.
It has no effect if GIT_AUTHOR_EMAIL is being processed correctly.
@wjt wjt merged commit 62d8c5f into flathub-infra:master Apr 30, 2024
3 checks passed
@dbnicholson dbnicholson deleted the no-clear-env branch April 30, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GIT_AUTHOR_NAME and GIT_AUTHOR_EMAIL filtered from environment
2 participants