feat: allow GIT_USER env var to be unset if SSH is used #5840

wpyoga · 2021-10-31T03:01:01Z

Motivation

Fixes #3454 and allows me to run yarn deploy with one less environment variable.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

I used this on my own GitHub Pages site, and the remote URL is git@github.com:wpyoga/wpyoga.github.io.git:

$ yarn add ../docusaurus_not-require-use-ssh/packages/docusaurus
$ yarn install
$ DEPLOYMENT_BRANCH=gh-pages yarn deploy

netlify · 2021-10-31T03:07:35Z

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 29ae674

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/618ba0f96a5f3c0008293460

😎 Browse the preview: https://deploy-preview-5840--docusaurus-2.netlify.app

github-actions · 2021-10-31T03:07:52Z

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟠 Performance	78
🟢 Accessibility	98
🟢 Best practices	100
🟢 SEO	100
🟢 PWA	95

Lighthouse ran on https://deploy-preview-5840--docusaurus-2.netlify.app/

packages/docusaurus/src/commands/deploy.ts

Co-authored-by: Joshua Chen <sidachen2003@gmail.com>

wpyoga · 2021-10-31T05:30:53Z

@Josh-Cena Thanks for the input :)

I wonder why the ESLint doesn't give a warning for this?

    currentRepoUrl.match(/^([\w-]+@)?[\w.-]+:[\w.\-/_]+(\.git)?/) !== null;

After the colon, we have:

[\w.\-/_]

Why does this hyphen need to be escaped?

Josh-Cena · 2021-10-31T05:57:57Z

[\w.\-/_]

Because without the escape it becomes a range mark, as in [A-Z]. If the dash is placed last then you don't need the escape either: [\w./_-]

I'm not confident to say that this PR is safe to merge although it looks pretty good to me. Please resolve the conflicts and we can wait for @slorber to review this :D

…onfig.js (facebook#5841) * feat: allow user to specify deploymentBranch property in docusaurus.config.js * docs: remove extra backtick * docs: fix broken code block

* docs: fix i18n routes to feature requests * Add redirect rules

Co-authored-by: Joshua Chen <sidachen2003@gmail.com>

Josh-Cena · 2021-10-31T06:43:29Z

Oops, shouldn't have force-pushed your branch😅 Anyways, fixed the branch.

…to not-require-use-ssh

wpyoga · 2021-10-31T06:52:18Z

Because without the escape it becomes a range mark, as in [A-Z]. If the dash is placed last then you don't need the escape either: [\w./_-]

Thank you, I've modified the regex :)

slorber

Thanks, that looks good to me overall, but not totally confident without running this myself.

Also, we have this code:

  const gitPass: string | undefined = process.env.GIT_PASS;
  let gitCredentials = `${gitUser}`;
  if (gitPass) {
    gitCredentials = `${gitCredentials}:${gitPass}`;
  }

Probably requires some refactoring, ensuring we don't have a 'undefined' string being passed to later method: I'd rather have a real undefined (non-string) value (even if it's ignored later in the process, it's more explicit.

Wouldn't it be easier if the doc recommended using a ssh URL? We could avoid adding the extra env variable in the commands, and only show them in another tab?

examples/classic-typescript/README.md

slorber · 2021-11-03T16:02:57Z

packages/docusaurus/src/commands/deploy.ts

+
+  const repoUrlUseSSH =
+    currentRepoUrl.match(/^ssh:\/\//) !== null ||
+    currentRepoUrl.match(/^([\w-]+@)?[\w.-]+:[\w./_-]+(\.git)?/) !== null;


Where do these regexp come from? If you found there somewhere I'd appreciate a link to an immutable URL as comment.

Can you add test cases for those?

This seems... intuitive to me🧐

It's either ssh://*** or user@server:project.git

A regexp is a less readable shorthand for real code and must be tested in the same way. Similarly an if/else is intuitive until there's a bug in it 🤪

Josh-Cena · 2021-11-07T09:51:40Z

After the refactoring the changes make a lot of sense: the git credentials aren't even used if using SSH. The only case I'm not sure of is inferring useSSH = true when the source repo is using SSH🧐 I think in most instances that's a valid assumption?

Josh-Cena

I believe this is safe. The deployment works like this:

The source repository is cloned. If the clone happens through SSH (the repo URL is SSH), it means there's already an SSH key associated in the environment to carry out all further interactions with GitHub.
The deploy command does the build.
In case of a cross-repo publish, the deployment repo is cloned & checked out. If using HTTPS, GH will ask for the username & password for authentication; if using SSH, the clone will always succeed (just like cloning the source repo). Note: the user here doesn't have to be the same that made the deployment commit. She just needs to have push access to the deployment repo.
The build output is committed to the deployment branch. It requires setting the git username & email locally, which is not handled by the deploy command itself. (I wonder if it's possible to also enable GPG signing)
The commit is pushed to the deployment origin. Already authenticated during the clone step—should always succeed.

The workflow is a bit messy and lacking explanations. I'll try to add some inline comments & refactor since we also have #5829 waiting to be merged

slorber

That looks fine but I haven't tested it in practice 😅 if there are errors we could patch it later anyway

Should this be merged before #5829 ? I don't know

slorber · 2021-11-09T17:51:06Z

packages/docusaurus/src/commands/deploy.ts

+    if (
+      process.env.USE_SSH === undefined &&
+      // ssh://***: explicit protocol
+      (/^ssh:\/\//.test(sourceRepoUrl) ||


what about having an external/tested hasSSHProtocol(sourceRepoUrl) fn?

Josh-Cena · 2021-11-10T10:23:22Z

Yeah, we can merge this and then I can test with canary myself... I don't see any clear pitfalls, but there may be corner cases

feat: allow GIT_USER env var to be unset if SSH is used

429ee29

wpyoga requested review from lex111 and slorber as code owners October 31, 2021 03:01

facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 31, 2021

wpyoga mentioned this pull request Oct 31, 2021

feat: allow user to specify deploymentBranch property in docusaurus.config.js #5841

Merged

Josh-Cena reviewed Oct 31, 2021

View reviewed changes

packages/docusaurus/src/commands/deploy.ts Outdated Show resolved Hide resolved

Josh-Cena added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Oct 31, 2021

fix: packages/docusaurus/src/commands/deploy.ts

7f9fcf8

Co-authored-by: Joshua Chen <sidachen2003@gmail.com>

wpyoga and others added 4 commits October 31, 2021 13:36

feat: allow user to specify deploymentBranch property in docusaurus.c…

729f922

…onfig.js (facebook#5841) * feat: allow user to specify deploymentBranch property in docusaurus.config.js * docs: remove extra backtick * docs: fix broken code block

docs: fix i18n routes to feature requests (facebook#5843)

2fa5e81

* docs: fix i18n routes to feature requests * Add redirect rules

feat: allow GIT_USER env var to be unset if SSH is used

40049c7

fix: packages/docusaurus/src/commands/deploy.ts

4c94610

Co-authored-by: Joshua Chen <sidachen2003@gmail.com>

Josh-Cena force-pushed the not-require-use-ssh branch from 2fa5e81 to 4c94610 Compare October 31, 2021 06:42

This comment has been minimized.

Sign in to view

wpyoga added 2 commits October 31, 2021 13:49

fix: avoid escaping hyphen in regex

4f5d35e

Merge branch 'not-require-use-ssh' of github.com:wpyoga/docusaurus in…

739b317

…to not-require-use-ssh

This comment has been minimized.

Sign in to view

slorber requested changes Nov 3, 2021

View reviewed changes

Josh-Cena mentioned this pull request Nov 6, 2021

docs: update GitHub deployment instructions #5888

Merged

Merge branch 'main' into not-require-use-ssh

426fbd6

Refactor

e0234b7

Josh-Cena added 2 commits November 8, 2021 19:31

Merge branch 'main' into not-require-use-ssh

e72ee62

Update deployment.mdx

08c2247

Josh-Cena requested review from slorber and Josh-Cena November 9, 2021 01:31

Make SSH higher priority

7d25b55

Josh-Cena approved these changes Nov 9, 2021

View reviewed changes

Only infer but not override

c3fcacd

slorber approved these changes Nov 9, 2021

View reviewed changes

Add tests

fe821bd

Josh-Cena added 2 commits November 10, 2021 18:32

Fix tests

fb7760c

Fix

29ae674

Josh-Cena merged commit f5732e7 into facebook:main Nov 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow GIT_USER env var to be unset if SSH is used #5840

feat: allow GIT_USER env var to be unset if SSH is used #5840

wpyoga commented Oct 31, 2021 •

edited by Josh-Cena

netlify bot commented Oct 31, 2021 •

edited

github-actions bot commented Oct 31, 2021 •

edited

wpyoga commented Oct 31, 2021 •

edited

Josh-Cena commented Oct 31, 2021

Josh-Cena commented Oct 31, 2021

This comment has been minimized.

wpyoga commented Oct 31, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

slorber left a comment

slorber Nov 3, 2021

Josh-Cena Nov 7, 2021 •

edited

slorber Nov 9, 2021

Josh-Cena commented Nov 7, 2021

Josh-Cena left a comment •

edited

slorber left a comment •

edited

slorber Nov 9, 2021

Josh-Cena commented Nov 10, 2021

feat: allow GIT_USER env var to be unset if SSH is used #5840

feat: allow GIT_USER env var to be unset if SSH is used #5840

Conversation

wpyoga commented Oct 31, 2021 • edited by Josh-Cena

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

netlify bot commented Oct 31, 2021 • edited

github-actions bot commented Oct 31, 2021 • edited

wpyoga commented Oct 31, 2021 • edited

Josh-Cena commented Oct 31, 2021

Josh-Cena commented Oct 31, 2021

This comment has been minimized.

wpyoga commented Oct 31, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

slorber left a comment

Choose a reason for hiding this comment

slorber Nov 3, 2021

Choose a reason for hiding this comment

Josh-Cena Nov 7, 2021 • edited

Choose a reason for hiding this comment

slorber Nov 9, 2021

Choose a reason for hiding this comment

Josh-Cena commented Nov 7, 2021

Josh-Cena left a comment • edited

Choose a reason for hiding this comment

slorber left a comment • edited

Choose a reason for hiding this comment

slorber Nov 9, 2021

Choose a reason for hiding this comment

Josh-Cena commented Nov 10, 2021

wpyoga commented Oct 31, 2021 •

edited by Josh-Cena

netlify bot commented Oct 31, 2021 •

edited

github-actions bot commented Oct 31, 2021 •

edited

wpyoga commented Oct 31, 2021 •

edited

Josh-Cena Nov 7, 2021 •

edited

Josh-Cena left a comment •

edited

slorber left a comment •

edited