decaporg/decap-cms

Default to branch name `main` rather than `master`

mooeypoo opened this issue · 18 comments

mooeypoo commented

Describe the bug
The documents reference a default branch master; this should be changed to main if possible, especially seeing as the Netlify deployment documents seem to encourage that too, and github's changing its default to main as well.

To Reproduce

  1. https://www.netlifycms.org/docs/add-to-your-site/#backend
  2. See comment branch: master # Branch to update (optional; defaults to master)

Expected behavior
I'd expect the branch to default to main, or at least the documentation to acknowledge that's becoming the new default in most places.

erezrokah commented

Good point @mooeypoo, we usually show in the docs the default value, but I think we should use main in that case.
Would you like to submit a PR for it?

I think what we really want, is for the CMS to use the default branch instead of defaulting to master:
https://github.com/netlify/netlify-cms/blob/65939d978eedda86526dbafc829f8e662442f227/packages/netlify-cms-backend-github/src/implementation.tsx#L119

That would require some more work though. Also we would need to do it in a non breaking way:

  1. Check if there is a branch named master
  2. If there is one - use it (for backwards compatibility)
  3. Otherwise retrieve the default branch and use that one

All of the above should happen where no branch is configured.

WDYT?

mooeypoo commented

Good point @mooeypoo, we usually show in the docs the default value, but I think we should use main in that case.
Would you like to submit a PR for it?

I think what we really want, is for the CMS to use the default branch instead of defaulting to master:

Ah! I was hoping for that as well, but I didn't want to overstep and increase your workload for a feature I wasn't entirely sure about its complexity ;) I figured an initial fix (either 'main' in defaults, or even just recommending 'main' in the comment?) would be a good step forward.

That said -- I'm talking out of my usual expertise-bubble here, so I'm not actually sure, but doesn't github provide data about what is the default branch? I'm wondering if it's possible to use that instead of guessing too much (with a backwards compat condition). I might be wrong here, but if I remember correctly, deploying to netlify from github seemed to have auto-recognize my main branch correctly... am I misremembering or is there potential here?

In any case this:

https://github.com/netlify/netlify-cms/blob/65939d978eedda86526dbafc829f8e662442f227/packages/netlify-cms-backend-github/src/implementation.tsx#L119

That would require some more work though. Also we would need to do it in a non breaking way:

  1. Check if there is a branch named master
  2. If there is one - use it (for backwards compatibility)
  3. Otherwise retrieve the default branch and use that one

All of the above should happen where no branch is configured.

WDYT?

-- sounds good to me. I can take a look at submitting a PR for it.

Thanks!

erezrokah commented

Ah! I was hoping for that as well, but I didn't want to overstep and increase your workload for a feature I wasn't entirely sure about its complexity ;) I figured an initial fix (either 'main' in defaults, or even just recommending 'main' in the comment?) would be a good step forward.

Updating the docs is a great first improvement 💯

That said -- I'm talking out of my usual expertise-bubble here, so I'm not actually sure, but doesn't github provide data about what is the default branch? I'm wondering if it's possible to use that instead of guessing too much (with a backwards compat condition). I might be wrong here, but if I remember correctly, deploying to netlify from github seemed to have auto-recognize my main branch correctly... am I misremembering or is there potential here?

That is what I'm going for in step 3. in my comment.

mooeypoo commented

Ha, I misread your step 3 (probably because I was in a meeting, that'll teach me...)

I've started working on a PR, but I'm running into a couple of conceptual issues here.
(First, an admission-- this is my first foray into TypeScript, so excuse any wrong practices here)

I initially thought I'd need to create the method that fetches the default branch, but that method seems to already exist at https://github.com/netlify/netlify-cms/blob/65939d978eedda86526dbafc829f8e662442f227/packages/netlify-cms-backend-github/src/API.ts#L1184

(Added correction: I saw later that the above method isn't exactly what I need since it assumes this.branch exists already is the default; what I need is one to give me the default_branch property from Github. I can definitely create one -- but that is a good example of the problem I'm laying out below about changing the assumption of the code to an async result for this.branch property, so I am holding off on making any specific methods until I figure the part below out)

This method is async, though, so adding it to a process that resolves the branch name transforms this.branch property itself to have an asynchronous result -- something that the rest of the code seems to not quite expect, so I'm worried about other implications here.

A second problem I ran into is that I couldn't find a way to mock the result from github -- so even calling the methods api.getBranch and api.getDefaultBranch always resulted in undefined values, which ended up falling back on main (since the 'master' branch wasn't "found" because the result of that methods is always undefined). Not sure if I'm missing something here?

In any case, I am a little worried about the change to async and its potential implications on the rest of the code. I'm not 100% I'm not overcomplicating things here, though. Am I missing something that resolves this? Do we get general details earlier? Should I change the call but then go over all potential places that call the branch property and transform them to async too?

I can submit this as a PR but I wanted to pause and ask before I go through a much deeper change here in case my current direction might be misguided.

Here's what I have so far: I changed this.branch assignment to this:

    // Temporarily assign until resolved
    this.branch = config.backend.branch || 'main'; // this is for testing purposes to see what the unit tests return; it should be set to `master` eventually for backwards compatibility
    this.resolveDefaultBranchName(config.backend.branch).then(result => {
      this.branch = result;
    });

and then added the method to resolve the default branch name:

  async resolveDefaultBranchName(name = '') {
    if (name.trim()) {
      // TODO: If we're already here, we can also validate
      // that the branch name given in config exists,
      // and if it doesn't, we can treat it as if it wasn't given
      // so we fall back to the rest of the operation
      return name;
    }

    if (await this.api?.getBranch('master')) {
      // For backwards compatibility:
      // If no branch was given but 'master' exists, fall back on it
      return 'master';
    }

    // Get default branch, or fall back on 'main'
    const def = await this.api?.getDefaultBranch();
    return def?.name || 'main';
  }

I've added a test for resolveDefaultBranchName but it's failing due to the above problems.

bytrangle commented

Hello @mooeypoo . Are you still working on this issue?

mooeypoo commented

Hello @mooeypoo . Are you still working on this issue?

I was for a bit, and then drowned a bit at work and discovered I have much less time than I wanted...
If anyone wants to pick this up, please do! I don't think I'll have a lot of time to delve back into this unfortunately for a bit longer.

bytrangle commented

Hello @erezrokah . This is what you said in order to resolve branch when no branch name is specified in config:

  1. Check if there is a branch named master
  2. If there is one - use it (for backwards compatibility)
  3. Otherwise retrieve the default branch and use that one

All of the above should happen where no branch is configured.

I don't understand why step 1 and 2 are neccessary? Is it because there are already users who leave out branch configuration and their content have been pushed to master for a while? So if I just retrieve the default branch and use that one, the default branch may not be master and now their new content will be pushed to this not-master branch and hell will break lose?

erezrokah commented

I don't understand why step 1 and 2 are neccessary? Is it because there are already users who leave out branch configuration and their content have been pushed to master for a while? So if I just retrieve the default branch and use that one, the default branch may not be master and now their new content will be pushed to this not-master branch and hell will break lose?

💯

  1. and 2. are to not break the CMS for users that leave out the branch configuration and have their default branch not set to master
bytrangle commented
  1. and 2. are to not break the CMS for users that leave out the branch configuration and have their default branch not set to master

Masterfully explained 💯. Thank you. I don't have Gitlab or Bitbucket account but is it also an issue on those services? AFAIK they used to name default branch master , but have moved away from that, like Github.

erezrokah commented

I don't have Gitlab or Bitbucket account but is it also an issue on those services? AFAIK they used to name default branch master , but have moved away from that, like Github.

We'll need to verify, but using the same suggested logic like GitHub should work for GitLab and Bitbucket too, correct?

bytrangle commented

We'll need to verify, but using the same suggested logic like GitHub should work for GitLab and Bitbucket too, correct?

I haven't dived into it yet, but your suggestion sounds good. Will get back with what I find soon.

bytrangle commented

Hello @erezrokah. I've realized that I've only worked with the editor in Netlify CMS and thus never set up the app locally for debugging the backend.

If I understand correctly from this "Contributing" guide, I would need to add at least backend name and a legit repo path to config, like this:

// dev-test/config.yml
backend:
  name: github
  repo: bytrangle/netlify-cms-jekyll

Is it correct?

erezrokah commented

Is it correct?

Hi @bytrangle, looks like you already figured that one out in #5814

bytrangle commented

Hello @erezrokah . I still don't understand how to test all the local changes to the backend libraries for Github, Gitlab, Bitbucket etc.

Suppose I've done some changes to how the default branch is set for Github users. To see those changes, I'd adjust the config under dev-test:

backend:
  name: github
  repo: my-github-account/my-repo

site_url: https://site-where-my-github-repo-is-deployed.netlify.app/

Now, I need to make those same changes to Gitlab. I need to change the config to point to a valid Gitlab repo and a live site where that repo is deployed.

backend:
  name: gitlab
  repo: my-gitlab-account/my-repo

site_url: https://site-where-my-gitlab-repo-is-deployed.netlify.app/

After I finish testing with Gitlab, I repeat the steps above for Bitbucket and possibly Azure.

Is there a better way to do that?

erezrokah commented

Hi @bytrangle, that's the correct way to test backend changes.
You'll need to verify them on each service.

FYI, our automation uses pre-recorded data to test the backends, but that I'm not sure we need to update those tests for the linked PR.