Log website URL upon deployment #1329

KendrickAng · 2020-08-18T12:29:07Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [ ] Documentation update
• [ ] Bug fix
• [ ] New feature
• [X] Enhancement to an existing feature
• [ ] Other, please explain:

What is the rationale for this request?
This change helps the user easily access the website after deploying it using the MarkBind CLI.

What changes did you make? (Give an overview)
Github supports custom domains through the use of CNAME. However, to do so a file called CNAME must be placed in the root of the remote repository where we are deploying (the file is automatically created if one sets a manual deploy url through the github repo itself). Hence when markbind deploy is run, the following additional steps are done:

read the contents of the CNAME file in the origin remote with external library simple-git.
If not, there must be no custom url, and we proceed to manually construct the URL, first from the provided repo from site.json, otherwise using the remote itself.

Provide some example code that this change will affect:

 static getDeploymentUrl(options) {
    const HTTPS_PART = 'https://';
    const SSH_PART = 'git@github.com:';
    const GITHUB_IO_PART = 'github.io';

    // https://<name|org name>.github.io/<repo name>/
    function constructGhPagesUrl(remoteUrl) {
      if (remoteUrl.includes(HTTPS_PART)) {
        // https://github.com/<name|org>/<repo>.git (HTTPS)
        const parts = remoteUrl.split('/');
        const repoName = parts[parts.length - 1].toLowerCase();
        const name = parts[parts.length - 2].toLowerCase();
        return `https://${name}.${GITHUB_IO_PART}/${repoName}`;
      } else if (remoteUrl.includes(SSH_PART)) {
        // git@github.com:<name|org>/<repo>.git (SSH)
        const parts = remoteUrl.split('/');
        const repoName = parts[parts.length - 1].toLowerCase();
        const name = (parts[0].split(':'))[0];
        return `https://${name}.${GITHUB_IO_PART}/${repoName}`;
      }
      throw new Error(`Unknown remote url ${remoteUrl}`);
    }

    const { remote, branch, repo } = options;
    const cnamePm = gitUtil.getRemoteBranchFile('blob', remote, branch, 'CNAME');
    const remoteUrlPm = gitUtil.getRemoteUrl(remote);
    const promises = [cnamePm, remoteUrlPm];

    return Promise.all(promises)
      .then((results) => {
        const cname = results[0];
        const remoteUrl = results[1];
        if (cname) {
          return cname;
        } else if (repo) {
          return constructGhPagesUrl(repo);
        }
        return constructGhPagesUrl(remoteUrl);
      })
      .catch((err) => {
        logger.error(err);
        return null;
      });
  }

Testing instructions:

Create a dummy website using markbind init.
Run markbind deploy and check the url the website is deployed at.

Proposed commit message: (wrap lines at 72 characters)
Log website URL upon deployment

… jest [misc] update test case

ang-zeyu

package-lock (from running npm run setup) will need to be committed too

Some comments:

ang-zeyu · 2020-08-19T11:57:52Z

packages/core/src/utils/git.js

+  async getRemoteBranchFile(simpleGit, type, remote, branch, fileName) {
+    const catFileTarget = `${remote}/${branch}:${fileName}`;
+    return simpleGit.catFile([type, catFileTarget])
+      .catch((err) => {


since async is used here, let's do a try ... return await ... catch instead!

ang-zeyu · 2020-08-19T12:01:07Z

packages/core/test/unit/Site.test.js

@@ -27,6 +27,11 @@ jest.mock('fs');
 jest.mock('walk-sync');
 jest.mock('gh-pages');
 jest.mock('../../src/Page');
+jest.mock('simple-git', () => () => ({


is this mock still necessary?

@ang-zeyu otherwise, there would be tons of warnings warn: message=fatal: Not a valid object name origin/gh-pages:CNAME thrown in the test for deployment in Site.test.js.

ang-zeyu · 2020-08-19T12:09:59Z

packages/core/src/Site/index.js

+
+    // https://<name|org name>.github.io/<repo name>/
+    function constructGhPagesUrl(remoteUrl) {
+      if (remoteUrl.includes(HTTPS_PART)) {


should we use startsWith here instead?

Likewise for SSH_PART (i'm not too sure on the format requirements here though, correct me if I'm wrong!)

yes, that would make more sense, thank you!

ang-zeyu · 2020-08-19T12:11:26Z

packages/core/src/Site/index.js

+        const name = (parts[0].split(':'))[0];
+        return `https://${name}.${GITHUB_IO_PART}/${repoName}`;
+      }
+      throw new Error(`Unknown remote url ${remoteUrl}`);


Since the message here isn't used, let's just return null directly for use in the above.

depUrl => (depUrl != null ? resolve(`Deployed at ${depUrl}!`) : resolve('Deployed!'))

Could remove the catch(..) below as well

@ang-zeyu I'm not sure whether we should remove the catch(..) statement; I put it there to deal with cases where gitUtil fails when calling the methods of simple-git, causing Promise.all to reject

there's a try ... catch in gitUtil already though

okay, I've removed the throw statement; it returns null now

ang-zeyu · 2020-08-19T12:15:22Z

packages/core/src/Site/index.js

-        .then(resolve)
+        .then(() => {
+          const git = simpleGit({ baseDir: process.cwd() });
+          const options = {};


Is it possible to move the previous options up to avoid duplication?

I think we can use a more explicit variable name (maybe deployOptions?) to make it clearer what kind of options these are as well!

ang-zeyu · 2020-08-19T12:46:31Z

packages/core/src/utils/git.js

+   */
+  async getRemoteBranchFile(simpleGit, type, remote, branch, fileName) {
+    const catFileTarget = `${remote}/${branch}:${fileName}`;
+    return simpleGit.catFile([type, catFileTarget])


Not too familiar with CNAME file and cat-file, give me some time to take a look at that!
Perhaps one of the other maintainers @acjh @marvinchin might have across it before though

This isn't something I'm familiar with either, unfortunately :(

ang-zeyu · 2020-08-19T12:50:07Z

packages/core/src/Site/index.js

+        // git@github.com:<name|org>/<repo>.git (SSH)
+        const parts = remoteUrl.split('/');
+        const repoName = parts[parts.length - 1].toLowerCase();
+        const name = (parts[0].split(':'))[0];


should the ending be [1]? would parts[0].substring(SSH_PART.length) work instead?

yes, that would work better!

marvinchin

Thanks for looking at this! Left a couple of suggestions 🙂

marvinchin · 2020-08-19T20:30:40Z

packages/cli/index.js

-      .then(() => {
-        logger.info('Deployed!');
-      })
+      .then(deployMsg => logger.info(deployMsg))


It doesn't feel very intuitive for the Site constructor to return a message. The message feels more like the responsibility of the CLI. I think that it might be more appropriate for it to return some data about the site created (the url in this case), which we can then use to construct the deploy message here.

I see, I'll do that!

marvinchin · 2020-08-19T20:34:58Z

packages/core/src/Site/index.js

-        .then(resolve)
+        .then(() => {
+          const git = simpleGit({ baseDir: process.cwd() });
+          const options = {};


I think we can use a more explicit variable name (maybe deployOptions?) to make it clearer what kind of options these are as well!

marvinchin · 2020-08-19T20:41:08Z

packages/core/src/Site/index.js

+          options.branch = this.siteConfig.deploy.branch || defaultDeployConfig.branch;
+          return Site.getDeploymentUrl(git, options);
+        })
+        .then(depUrl => (depUrl != null ? resolve(`Deployed at ${depUrl}!`) : resolve('Deployed!')))


Can we use !== instead of !=?

I didn't notice I put !=, thanks!

marvinchin · 2020-08-19T20:43:07Z

packages/core/src/Site/index.js

+    }
+
+    const { remote, branch, repo } = options;
+    const cnamePm = gitUtil.getRemoteBranchFile(git, 'blob', remote, branch, 'CNAME');


Maybe I'm missing some context here, but I'm not sure what Pm stands for. Is it for promise?

If it is, I'd prefer to avoid using uncommon (to me, at least! if this is a convention you are more familiar with please correct me) abbreviations to avoid confusion when future devs look at this variable.

It was meant to be shorthand for 'Promise'; Ill rename it to cnamePromise for better readability. Thanks!

marvinchin · 2020-08-19T20:45:30Z

packages/core/src/utils/git.js

+   */
+  async getRemoteBranchFile(simpleGit, type, remote, branch, fileName) {
+    const catFileTarget = `${remote}/${branch}:${fileName}`;
+    return simpleGit.catFile([type, catFileTarget])


This isn't something I'm familiar with either, unfortunately :(

ang-zeyu

Looks good code quality wise!

Some minor implementation issues:

Hmm seems like there's a newline after the deployment url if it is from cname sometimes; Let's strip trailing whitespace / newlines just in case

Let's remove the logger.warn in gitUtil, since the user can use either option

- remote logger.warn from gitUtils - trim whitespaces and line endings from cname

KendrickAng · 2020-08-22T12:23:45Z

okay, I've made the changes as requested!

ang-zeyu · 2020-08-22T14:36:59Z

packages/cli/package-lock.json

@@ -4,3052 +4,3023 @@
 	"lockfileVersion": 1,
 	"requires": true,
 	"dependencies": {
-		"jest": {


there shouldn't be any diffs in this package-lock; likely from a npm install

#1329 (comment) as well!

rectified the changed package-lock.json!

ang-zeyu

Sorry for the wait

I should really have untagged this as good first issue given the amount of additions in #947 (oh well =X)
Thanks! And well done for a first issue : )

Kendrick Ang and others added 6 commits August 15, 2020 21:31

[wip] add git wrapper

e644603

implement get deploy url

19cbd5f

restructure Site/index.js, fix lint errors

5657f3e

Merge branch 'master' into feature/MB-844

e3ea592

add base dir when initializing simple-git

024868c

[fix] No such file or directory error when initializing simple-git in…

6e249cb

… jest [misc] update test case

ang-zeyu reviewed Aug 19, 2020

View reviewed changes

marvinchin requested changes Aug 19, 2020

View reviewed changes

KendrickAng added 2 commits August 21, 2020 23:40

add package-lock.json

dccad64

update code from pr comments

053ee1b

ang-zeyu requested changes Aug 22, 2020

View reviewed changes

KendrickAng added 2 commits August 22, 2020 20:14

update package-lock.json

ce58556

make changes from comments

431613a

- remote logger.warn from gitUtils - trim whitespaces and line endings from cname

ang-zeyu reviewed Aug 22, 2020

View reviewed changes

KendrickAng added 2 commits August 23, 2020 16:58

update package-lock.json

d2f7c32

update package-lock.json

ac0066f

ang-zeyu approved these changes Aug 27, 2020

View reviewed changes

ang-zeyu added this to the v2.15.3 milestone Aug 27, 2020

ang-zeyu approved these changes Aug 27, 2020

View reviewed changes

ang-zeyu merged commit 08ef1ee into MarkBind:master Aug 27, 2020

KendrickAng mentioned this pull request Sep 27, 2020

Fix error when deploying #1368

Merged

ang-zeyu mentioned this pull request Oct 8, 2020

Log website url on MarkBind deploy #844

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log website URL upon deployment #1329

Log website URL upon deployment #1329

KendrickAng commented Aug 18, 2020

ang-zeyu left a comment

ang-zeyu Aug 19, 2020

ang-zeyu Aug 19, 2020

KendrickAng Aug 21, 2020

ang-zeyu Aug 19, 2020

KendrickAng Aug 21, 2020

ang-zeyu Aug 19, 2020

KendrickAng Aug 21, 2020 •

edited

Loading

ang-zeyu Aug 22, 2020

KendrickAng Aug 23, 2020

ang-zeyu Aug 19, 2020

marvinchin Aug 19, 2020

ang-zeyu Aug 19, 2020

marvinchin Aug 19, 2020

ang-zeyu Aug 19, 2020

KendrickAng Aug 21, 2020

marvinchin left a comment

marvinchin Aug 19, 2020

KendrickAng Aug 21, 2020

marvinchin Aug 19, 2020

marvinchin Aug 19, 2020

KendrickAng Aug 21, 2020

marvinchin Aug 19, 2020

marvinchin Aug 19, 2020

KendrickAng Aug 21, 2020

marvinchin Aug 19, 2020

ang-zeyu left a comment

KendrickAng commented Aug 22, 2020 •

edited

Loading

ang-zeyu Aug 22, 2020

ang-zeyu Aug 22, 2020

KendrickAng Aug 23, 2020

ang-zeyu left a comment

Log website URL upon deployment #1329

Log website URL upon deployment #1329

Conversation

KendrickAng commented Aug 18, 2020

ang-zeyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KendrickAng Aug 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marvinchin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ang-zeyu left a comment

Choose a reason for hiding this comment

KendrickAng commented Aug 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ang-zeyu left a comment

Choose a reason for hiding this comment

KendrickAng Aug 21, 2020 •

edited

Loading

KendrickAng commented Aug 22, 2020 •

edited

Loading