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

Handle GPG sign git config for initial commit #5823

Merged
merged 2 commits into from Aug 27, 2020
Merged

Handle GPG sign git config for initial commit #5823

merged 2 commits into from Aug 27, 2020

Conversation

spenserblack
Copy link
Contributor

@spenserblack spenserblack commented Aug 25, 2020

Fixes #5818

If the git config option commit.gpgSign is set
to true, passes the flag --no-gpg-sign when
creating the initial commit, then warns user that
initial commit is not signed and instructs how
to sign the initial commit

If git config commit.gpgSign = true, then warning on failed initial commit is expanded to also state that failure to commit could
be due to failure to sign commit. Also sets the commit.gpgSign config to false if testing or debugging.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Underlying tools
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

Other information:
This PR will change vue create to never sign the initial commit. My personal opinion is that this is the preferred behavior, since I wouldn't want to default to signing a commit that I did not create. But another way to fix #5818 would be to continue to attempt GPG sign if commit.gpgSign = true in git config, and make warning say that commit failed due to missing username, email, or failure to sign commit. If you think it is better to continue signing and change the gitCommitFailed warning instead, I could create a PR for that instead 🙂

@sodatea
Copy link
Member

sodatea commented Aug 26, 2020

I prefer to change the error message.
Because:

  1. even after this PR, the error message is still inaccurate, as the commit may fail for other reasons such as wrong password
  2. it's simpler to change the error message, thus more maintainable

@spenserblack
Copy link
Contributor Author

spenserblack commented Aug 26, 2020

I prefer to change the error message.

No problem 👍
Would you prefer I open a new PR, or edit this PR?

Question: Should the error message be something like this?

`Skipped git commit due to missing username and email in git config${gpgSign === 'true' ? ' or failed to sign commit' : '' }.`

@sodatea
Copy link
Member

sodatea commented Aug 26, 2020

I think either is fine.

The message looks good to me.

@spenserblack
Copy link
Contributor Author

👍

Should I also add this to prevent GPG signing on testing/debugging?

 if (isTestOrDebug) {
   await run('git', ['config', 'user.name', 'test'])
   await run('git', ['config', 'user.email', 'test@test.com'])
+  await run('git', ['config', '--unset', 'commit.gpgSign'])
 }

@sodatea
Copy link
Member

sodatea commented Aug 26, 2020

Yeah, that would be nice.

If git config `commit.gpgSign` is `true`, warning
will be expanded to also say that failed commit
may be due to failure to sign commit.

Addresses #5818
@spenserblack spenserblack changed the title fix(initial commit): default to no GPG sign Handle GPG sign git config for initial commit Aug 26, 2020
Sets git config `commit.gpgSign` on test or
debug to false in order to stop git from
attempting to sign commit with a GPG signature
that does not belong to test@test.com

Addresses #5818
@spenserblack
Copy link
Contributor Author

 if (isTestOrDebug) {
   await run('git', ['config', 'user.name', 'test'])
   await run('git', ['config', 'user.email', 'test@test.com'])
+  await run('git', ['config', 'commit.gpgSign', 'false'])
 }

Did this instead because --unset fails if the config option was never set.

Copy link
Member

@sodatea sodatea left a comment

Choose a reason for hiding this comment

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

Thanks!

@sodatea sodatea merged commit 9706cd1 into vuejs:dev Aug 27, 2020
@char101
Copy link

char101 commented Sep 10, 2020

git config --get returns 1 if the config is not found. This is treated as error by execa. The execution should be wrapped in an exception handler.

�  Generating README.md...
 ERROR  Error: Command failed: git config --get commit.gpgSign


Error: Command failed: git config --get commit.gpgSign


    at makeError (C:\Lang\node\global\node_modules\@vue\cli\node_modules\execa\index.js:174:9)
    at C:\Lang\node\global\node_modules\@vue\cli\node_modules\execa\index.js:278:16
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async C:\Lang\node\global\node_modules\@vue\cli\lib\Creator.js:271:43
    at async Creator.create (C:\Lang\node\global\node_modules\@vue\cli\lib\Creator.js:270:17)
    at async create (C:\Lang\node\global\node_modules\@vue\cli\lib\create.js:72:3)

@sodatea
Copy link
Member

sodatea commented Sep 10, 2020

@char101 Thanks for the feedback! I'll fix this ASAP.

sodatea added a commit to sodatea/vue-cli that referenced this pull request Sep 10, 2020
Fix the issue described at vuejs#5823 (comment)

This simplifies the logic, thus less error-prone.
The error message is still correct anyway.
@spenserblack
Copy link
Contributor Author

spenserblack commented Sep 10, 2020

😨 I apologize for this! I didn't notice this behavior in my environment. I should have also tested in an environment where the option was never set.

@spenserblack spenserblack deleted the bugfix/gpg-sign branch September 10, 2020 13:15
sodatea added a commit that referenced this pull request Sep 10, 2020
Fix the issue described at #5823 (comment)

This simplifies the logic, thus less error-prone.
The error message is still correct anyway.
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.

Incorrect warning on failed commit after vue create
3 participants