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

refactor: optimize clone and checkout in deploy command #5829

Merged

Conversation

sivapalan
Copy link
Contributor

Motivation

The fix in #5828 was probably the safest and quickest solution for the bug that was causing either deployment failures or overwriting of commit history. But, as mentioned in that PR description, an alternative and more optimized solution would be to specify --branch ${deploymentBranch} for the clone command and thereby skip the checkout step altogether (except for the initial deployment where an orphan branch will have to be checked out).

Let me know if there are any edge cases I've missed or any side effects that should be taken into consideration for this change.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Tested command locally.

Related PRs

#5828

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 29, 2021
@netlify
Copy link

netlify bot commented Oct 29, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: a2e29b0

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

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

@github-actions
Copy link

github-actions bot commented Oct 29, 2021

⚡️ Lighthouse report for the changes in this PR:

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

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

@slorber
Copy link
Collaborator

slorber commented Oct 29, 2021

This kind of code is not so easy to review for me. Can you describe exactly what I should do locally to test this?

@sivapalan
Copy link
Contributor Author

This kind of code is not so easy to review for me. Can you describe exactly what I should do locally to test this?

  1. Clone the repo and check out the deployment branch:
    git clone --depth 1 --branch <DEPLOYMENT BRANCH> <REPO URL>

    If this command fails (expected when the specified deployment branch doesn't exist), go to step 2. Otherwise, you can skip step 2 and go directly to step 3.

  2. This step should only be necessary for the initial deployment on the specified deployment branch, as that should be the only situation where the branch doesn't exist yet and the clone command in step 1 fails.

    • Clone the repository's default branch:
      git clone --depth 1 <REPO URL>
    • cd to the cloned directory
    • If the specified deployment branch is different from the default branch, check out an orphan branch with the specified name:
      git checkout --orphan <DEPLOYMENT BRANCH>
  3. The remaining commands should be the same as before (with the exception of the cd command which has been moved):

    • cd to the cloned directory (this has no effect if you've already done this in step 2)
    • git rm -rf .
    • Copy the files you want to deploy to the current directory
    • git add -all
    • git commit -m "<COMMIT MESSAGE>"
    • git push --force origin <DEPLOYMENT BRANCH>

@Josh-Cena Josh-Cena added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Oct 30, 2021
@sivapalan
Copy link
Contributor Author

I think this can be further simplified by removing the defaultBranch !== deploymentBranch check, which seems to be redundant after the refactoring. The code could then look like this (compacted for brevity):

if (shellExecLog(`git clone --depth 1 --branch ${deploymentBranch} ${remoteBranch} ${toPath}`).code == 0) { // Attempt cloning specified deployment branch
  shell.cd(toPath); // If deployment branch clone is successful, cd to directory
} else if (shellExecLog(`git clone --depth 1 ${remoteBranch} ${toPath}`).code == 0) { // If deployment branch clone fails, attempt cloning repo's default branch
  shell.cd(toPath); // If default branch clone is successful, cd to directory
  if (shellExecLog(`git checkout --orphan ${deploymentBranch}`).code !== 0) { // Check out an orphan branch for deployment
    throw new Error(`Running "git checkout ${deploymentBranch}" command failed.`); // Throw error if checkout command fails
  }
} else {
  throw new Error(`Running "git clone" command in "${toPath}" failed.`); // Throw error if both clone commands fail
}

If no errors have been thrown, these prerequisites are now completed (and it should be ready to proceed with the subsequent steps):

  • cloned repo
  • cd to repo
  • checked out deployment branch

Current code that achieves the same:

if (
shellExecLog(
`git clone --depth 1 --no-single-branch ${remoteBranch} ${toPath}`,
).code !== 0
) {
throw new Error(`Running "git clone" command in "${toPath}" failed.`);
}
shell.cd(toPath);
// If the default branch is the one we're deploying to, then we'll fail
// to create it. This is the case of a cross-repo publish, where we clone
// a github.io repo with a default branch.
const defaultBranch = shell
.exec('git rev-parse --abbrev-ref HEAD')
.stdout.trim();
if (defaultBranch !== deploymentBranch) {
if (shellExecLog(`git checkout origin/${deploymentBranch}`).code !== 0) {
if (
shellExecLog(`git checkout --orphan ${deploymentBranch}`).code !== 0
) {
throw new Error(
`Running "git checkout ${deploymentBranch}" command failed.`,
);
}
} else if (
shellExecLog(`git checkout -b ${deploymentBranch}`).code +
shellExecLog(
`git branch --set-upstream-to=origin/${deploymentBranch}`,
).code !==
0
) {
throw new Error(
`Running "git checkout ${deploymentBranch}" command failed.`,
);
}
}

@Josh-Cena
Copy link
Collaborator

@sivapalan Are you going to do the refactor yourself, since it seems you do have a clear plan in mind? I also suggest you add some inline comments explaining each step so it will be easier for the reviewers and future code readers.

@sivapalan
Copy link
Contributor Author

@sivapalan Are you going to do the refactor yourself, since it seems you do have a clear plan in mind? I also suggest you add some inline comments explaining each step so it will be easier for the reviewers and future code readers.

I was just waiting for some input from you guys, regarding if this refactoring is wanted or not before doing any further work on this 🙂

I think there's also another additional simplification that can be done, but not sure if it's preferred or not. If the git clone --branch ${deploymentBranch} [...] command fails, we could assume that the deployment branch doesn't exist. So instead of then cloning the repository, checking out an orphan branch and removing all files, we could just initialize git in an empty directory, check out a clean deployment branch and add remote. This step will then not be able to catch f.ex. any authentication errors, so that will then be deferred until running git push.

This is now done in the latest commit: 63334e2

I've not added any error handling for the newly added commands:

  • git init
  • git checkout -b ${deploymentBranch}
  • git remote add origin ${remoteBranch}

Can these be considered "safe" as they don't do any remote calls, or should we add checks for the exit codes before continuing?

@Josh-Cena
Copy link
Collaborator

The idea looks very good to me, and it makes the code much easier to understand. 👍

I don't think we need error handling in those three commands. We don't do it for shellExecLog('git add --all'); either, so I think we are fine. I'm going to merge this and test it myself

@Josh-Cena
Copy link
Collaborator

In the future @slorber if we have a Docusaurus org, we should make a few site fixture repos and test E2E on those fixtures, including deploy

@Josh-Cena Josh-Cena merged commit 6c0a193 into facebook:main Nov 11, 2021
@sivapalan sivapalan deleted the optimize_deploy_command_clone_and_checkout branch November 11, 2021 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants