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

implement GPG signing behavior #78

Merged
merged 12 commits into from
Aug 1, 2021
Merged

Conversation

chebbyChefNEQ
Copy link
Contributor

This PR implements GPG signing behavior in ghstack.

This implementation mimics the behavior of git.

This PR resolves #77

@chebbyChefNEQ
Copy link
Contributor Author

chebbyChefNEQ commented Jul 24, 2021

Haven't had the time to set up a test yet. I need to set up git config file generation and use the GIT_CONFIG=<path> envvar to control it. Directly invoking git config just has too much potential for polluting the host env.

try:
# Why the complicated compare
# https://git-scm.com/docs/git-config#Documentation/git-config.txt-boolean
should_sign = shell.git("config", "--get", "commit.gpgsign") in ("yes", "on", "true", "1")
Copy link
Owner

Choose a reason for hiding this comment

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

Probably should cache this so we don't keep whacking git config

@ezyang
Copy link
Owner

ezyang commented Jul 24, 2021

This looks pretty good! A test would be great.

@chebbyChefNEQ
Copy link
Contributor Author

@ezyang Added a test to make sure are are adding -S when commit.gpgsign=true.

# Why the complicated compare
# https://git-scm.com/docs/git-config#Documentation/git-config.txt-boolean
_should_sign = shell.git("config", "--get", "commit.gpgsign") in ("yes", "on", "true", "1")
print(_should_sign)
Copy link
Owner

Choose a reason for hiding this comment

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

leftover debug printf

test_ghstack.py Outdated
])
# Set the env var
config = os.environ.get("GIT_CONFIG", None)
os.environ["GIT_CONFIG"] = f.name
Copy link
Owner

Choose a reason for hiding this comment

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

Modifying os.environ is generally frowned upon because it is global state that affects the rest of the process. So it would be better if os.environ was as an explicit argument to main(), so that we can more easily replace its contents when testing (as is the case here). Unfortunately, this is not the case, and I shouldn't really be asking you to do the free work of refactoring all of those sites haha. So I'll probably just take this as is.

@ezyang
Copy link
Owner

ezyang commented Aug 1, 2021

old test for future reference

ghstack/test_ghstack.py

Lines 2086 to 2125 in bb84c55

def test_gpgsign_should_fail_if_no_default_key(self) -> None:
print("####################")
print("### test_gpgsign_should_fail_if_no_default_key")
print("###")
@contextlib.contextmanager
def signing_enabled() -> Iterator[None]:
# Create a config file that enables signing
with tempfile.NamedTemporaryFile(delete=False) as f:
f.writelines(f"{x}\n".encode() for x in [
"[commit]",
"\tgpgsign=true",
])
# Set the env var
config = os.environ.get("GIT_CONFIG", None)
os.environ["GIT_CONFIG"] = f.name
# HACK: Since we cache the sign config, we need to manaully clear the cache
import ghstack.gpg_sign
ghstack.gpg_sign._should_sign = None
try:
yield
finally:
# Clean up
if config is None:
del os.environ["GIT_CONFIG"]
else:
os.environ["GIT_CONFIG"] = config
# HACK: Reset the config again, because we changed the config
ghstack.gpg_sign._should_sign = None
# Make a commit WITHOUT signing, we are testing if we are correctly injecting '-S' during submit
print("### Try to commit")
self.writeFileAndAdd("a", "asdf")
self.sh.git("commit", "-m", "Commit 1\n\nThis is my first commit")
with signing_enabled():
self.assertRaisesRegex(RuntimeError,
r"^(git commit-tree -S -p)( )[0-9a-f]{40}( )[0-9a-f]{40} (failed with exit code )[0-9]{1,3}$",
lambda: self.gh())



def gpg_args_if_necessary(
shell: ghstack.shell.Shell = ghstack.shell.Shell()
Copy link
Owner

Choose a reason for hiding this comment

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

once we have some sort of env plumbing, would feed env into this function and that would make it possible to override this setting in testing

@ezyang
Copy link
Owner

ezyang commented Aug 1, 2021

follow up issue at #80

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.

GPG signed commits
2 participants