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

Default to branch name main rather than master #5617

Closed
mooeypoo opened this issue Jul 15, 2021 · 18 comments · Fixed by #5844
Closed

Default to branch name main rather than master #5617

mooeypoo opened this issue Jul 15, 2021 · 18 comments · Fixed by #5844
Labels
area: extensions/backends type: feature code contributing to the implementation of a feature and/or user facing functionality

Comments

@mooeypoo
Copy link

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.

@mooeypoo mooeypoo added the type: bug code to address defects in shipped code label Jul 15, 2021
@erezrokah erezrokah added type: feature code contributing to the implementation of a feature and/or user facing functionality area: extensions/backends and removed type: bug code to address defects in shipped code labels Jul 15, 2021
@erezrokah
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

erezrokah commented Jul 15, 2021

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
Copy link
Author

mooeypoo commented Jul 17, 2021

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
Copy link
Collaborator

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

@mooeypoo
Copy link
Author

mooeypoo commented Sep 5, 2021

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator

  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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

bytrangle commented Sep 15, 2021

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
Copy link
Contributor

Is it correct?

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

@bytrangle
Copy link
Collaborator

bytrangle commented Sep 30, 2021

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
Copy link
Contributor

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.

@alice-lam
Copy link

Hi, was curious on the status of this task; We are also using Decap in an enterprise that flags us for uninclusive language so having master replaced to main would be very helpful.

@martinjagodic
Copy link
Member

The default branch has been renamed from master to main. In the docs this was done a while ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: extensions/backends type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants