Fix: Set correct branch when it's not specified in the config #5844

bytrangle · 2021-09-27T09:27:00Z

Summary
Currently, the default branch in Github, Gitlab and Bitbucket is set to be main instead of master.

However, when people use Netlify CMS and don't specify the branch name to push content in the config file, Netlify CMS will automatically push to branch master.

Since this branch may not exist, users will get an error on the frontend: "API_ERROR: Branch not found".

Test plan

In the admin/config.yml file, specify a valid Github repo path to push content to.
Run yarn start.
Go to Neltify CMS dashboard, create a new post and hit Publish. You shouldn't get an error. Checking back on the repo that you specified in the config, you should see the new post.

Checklist

Please add a x inside each checkbox:

I have read the contribution guidelines.
Code is formatted via running yarn format.
Tests are passing via running yarn test.
The status checks are successful (continuous integration). Those can be seen below.

bytrangle · 2021-09-29T14:44:38Z

Please hold off on reviewing until I've pushed the unit tests.

erezrokah

Thanks @bytrangle, great work as always.

I've added some comments, please let me know your thoughts.
Also, are we planning to do this for other backends too?

erezrokah · 2021-09-30T11:13:16Z

packages/netlify-cms-backend-github/src/implementation.tsx

+      if (!masterBranch) {
+        // Get default branch of the repo
+        const defaultBranch = await this.api!.newGetDefaultBranch();
+        this.branch = defaultBranch || '';


Suggested change

this.branch = defaultBranch || '';

this.branch = defaultBranch || 'master';

Should we default to master to ensure the property is set (this will avoid calling setDefaultBranch again if authenticate is re-invoked).

Two things:

I didn't consider the scenario that authenticate can be revoked. I will change it as requested.Good point.

I was going to apply the same approach to other backend, but then I realized that would mean writing some repetitive code. That will open up more room for errors and make it harder to track changes in the future. So what do you think about adding a helper in Netlify Utils to get a default branch and call that in each respective backend?

2. Netlify Utils to get a default branch and call that in each respective backend?

Sounds good! How about adding the helpers to https://github.com/netlify/netlify-cms/blob/master/packages/netlify-cms-lib-util/src/backendUtil.ts?

Should it be backendUtils or API.ts is the place?
And when do you need these changes? I normally don't work on open source on weekend, but if you need it soon, I can work on this issue on Sunday.

Either backendUtils or API.ts work (or both) depending on the common logic you extract.

I normally don't work on open source on weekend, but if you need it soon, I can work on this issue on Sunday.

I definitely don't need it soon, and in any case no need to change your work schedule for us. We are the ones who should adjust our schedule to contributors.

packages/netlify-cms-backend-github/src/implementation.tsx

erezrokah · 2021-09-30T11:14:55Z

packages/netlify-cms-backend-github/src/API.ts

@@ -1188,6 +1189,16 @@ export default class API {
    return result;
  }

+  async newGetDefaultBranch() {
+    try {


How about we drop the try/catch and let the calling function handle any errors (it already has a try/catch)?

This requestConfig object will be passed to a helper for making API request

Include switch clause to construct API urls for different backends

…ured The property is needed so that we'll only set default branch when branch prop is missing in config

…config This is needed so that this property won't be empty when authorization is revoked.

Reason: Requesting information from a public repo doesn't require token

bytrangle · 2021-10-12T09:21:58Z

Sorry that it took such a long time to update this PR. Writing a helper that can accommodate all the major git hosts turn out to be more complex than I had expected.

bytrangle · 2021-10-27T09:59:55Z

Hello @erezrokah . Let me ask a noob question: How do I rerun the Github action workflow? Or is it only possible for team members?

erezrokah · 2021-10-27T10:26:27Z

No noob questions - do you have this button when you open the action?

erezrokah · 2021-10-27T10:27:53Z

Regardless, I invited you to collaborate on the repo, long overdue ❤️

bytrangle · 2022-04-13T10:07:49Z

@erezrokah Thank you for the reformat work. I was doing it but you're so fast.

erezrokah

Thanks for following up with these changes @bytrangle, I added some comments.

Also, in order for this change to be backwards compatible we would need to default to master if exists (default or not), and only if it doesn't exist use the default branch.

Please let me know what you think

erezrokah · 2022-04-13T11:45:34Z

packages/netlify-cms-backend-bitbucket/src/implementation.ts

+        token: this.token,
+      });
+      if (defaultBranchName) {
+        this.branch = defaultBranchName;


Isn't setting this.branch here is too late? We already passed this.branch to the API ctor a few lines back https://github.com/netlify/netlify-cms/pull/5844/files#diff-09a7e7be36a8f9bbf96a0407a90bee6df174fef8477a6d68de3472067a884643R199

Indeed, I kept wrestling with the question of when to set this.branch. I put it after the check for user having write access because I thought there was no point setting branch when user is unauthenticated or doesn't have write access.

The restoreUser seems to be the earliest function called in the implementation module, followed by authenticate. Should I call getDefaultBranchName before setting up API in authenticate, or do you recommend another location?

erezrokah · 2022-04-13T11:45:58Z

packages/netlify-cms-backend-github/src/implementation.tsx

+        token: this.token,
+      });
+      if (defaultBranchName) {
+        this.branch = defaultBranchName;


See my previous comment about setting this.branch too late

erezrokah · 2022-04-13T11:46:25Z

packages/netlify-cms-backend-gitlab/src/implementation.ts

+        token: this.token,
+      });
+      if (defaultBranchName) {
+        this.branch = defaultBranchName;


Same comment as other backends

erezrokah · 2022-04-13T11:54:53Z

packages/netlify-cms-lib-util/src/API.ts

+  }
+}
+
+export async function getDefaultBranchName(configs: {


What do you think about having a getDefaultBranchName per backend API. The logic seems to be very specific for each API, and that way we won't need to duplicate the error handling, response parsing, etc. (as each API already handles it).

I know we discussed this here, but having the various APIs paths duplicated in the common lib is not a pattern we'd like to follow.

The common logic in this PR is the one that checks if a branch is configured and then fetches the default one if needed. I think it's quite small, so I don't see a reason to de-duplicate it.

What are your thoughts on this?

Yes, that's a good point. I think I was doing that (duplicate getDefaultBranchName per backend) in the draft. Then I thought maybe I would make it a little more efficient by putting it in the lib. But it wasn't.

stale · 2023-04-26T09:26:38Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

martinjagodic · 2023-10-16T09:49:42Z

@bytrangle are you still interested in moving this forward?

paulRbr · 2024-02-28T09:55:49Z

I think this PR is interesting to be merged upstream. I was going to suggest a much more simple change (changing the default value of the branch to main) but I think this PR is better.

martinjagodic · 2024-02-28T11:53:08Z

@paulRbr yes, we would like to have this merged, but we need to solve those merge conflicts first.

bytrangle · 2024-03-14T13:00:49Z

@martinjagodic Hi, I'm interested in working on this again. However, I may have to ask you about resolving merging conflicts later :).

martinjagodic · 2024-03-14T13:03:15Z

@bytrangle awesome! The conflicts emerged when we renamed the packages from netlify-cms-* to decap-cms-*. If a rename does not suffice, I am happy to answer questions/help out.

bytrangle · 2024-04-01T10:08:00Z

Hi @demshy . Is there anything else I need to do?

demshy · 2024-04-02T07:04:26Z

Sorry @bytrangle I somehow overlooked this one. I added a missing import that made the unit tests fail. I will gladly merge this one into a release this week

bytrangle added 4 commits September 27, 2021 11:46

feat: add helper for fetching default branch from Github

47ebd16

feat: add method for setting default branch

61904c8

fix: set default branch after user has authenticated successfully

c3f1fc0

fix: format code

90c9f9c

bytrangle requested a review from a team September 27, 2021 09:27

erezrokah added the type: bug code to address defects in shipped code label Sep 29, 2021

feat: add unit test for getting default branch name

43fbe2d

erezrokah suggested changes Sep 30, 2021

View reviewed changes

bytrangle added 17 commits October 11, 2021 12:02

feat: add helpers for parsing API responses

8436443

feat(lib-util): add helper for constructing request headers

3edc3ce

feat(lib-util): add helper for constructing full URL for API request

7ccbac6

feat(lib-util): store base URLs for each backend

e5407c3

feat(lib-util): add type annotation for the request config

3f0e711

This requestConfig object will be passed to a helper for making API request

feat(lib-util): add helper for handle API request error

a506b5d

feat(lib-util): add config for making api request

cc5f53a

feat(lib-util): add api request generator

cec8fbb

feat(lib-util): add helper for getting default branch name

69d124a

Include switch clause to construct API urls for different backends

feat(lib-util): export method for getting default branch name

c726ee0

feat(gh-backend): add a boolean property to check if branch is config…

bbafe77

…ured The property is needed so that we'll only set default branch when branch prop is missing in config

feat(gh-backend): set prop branch as master when it's missing in …

11e187c

…config This is needed so that this property won't be empty when authorization is revoked.

feat(gh-backend): set branch name when it's missing in config

67904b1

feat(gitlab-backend): set branch when it's not in the config

37531e1

feat(bitbucket-backend): set branch when it's not specified in config

b118aea

feat(lib-util): allow token type to be undefined

fa8e03c

Reason: Requesting information from a public repo doesn't require token

fix: format codes

a2add4a

style: format files

1789e6f

erezrokah suggested changes Apr 13, 2022

View reviewed changes

bytrangle added 7 commits May 17, 2022 13:58

fix: query branch name before setting Github API service

34375f4

fix: reformat implementation module of Github backend

7f6de72

fix: remove importing of getDefaultBranchName from lib

ec0ff89

fix: removed test for getDefaultBranchName in lib packages

48e7175

fix: removed unused vars in api test for lib package

676631f

feat: retrieve default branch before creating Bitbucket AI instance

614a5b0

fix: reformat codes in Bitbucket implementation module

20caf5f

stale bot added the status: stale label Apr 26, 2023

martinjagodic removed the status: stale label Apr 28, 2023

fix: Resolve conflicts from changing cms

8db27d6

bytrangle requested a review from a team as a code owner March 18, 2024 09:55

Merge branch 'master' into fix/default-branch

ad161df

martinjagodic requested review from demshy and removed request for a team March 18, 2024 13:00

demshy added 2 commits April 2, 2024 08:43

Merge branch 'main' into fix/default-branch

d4e4fe3

fix: add missing import

c6b05f8

demshy approved these changes Apr 2, 2024

View reviewed changes

Merge branch 'main' into fix/default-branch

5827471

demshy merged commit c91a70f into decaporg:main Apr 3, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Set correct branch when it's not specified in the config #5844

Fix: Set correct branch when it's not specified in the config #5844

bytrangle commented Sep 27, 2021

bytrangle commented Sep 29, 2021

erezrokah left a comment

erezrokah Sep 30, 2021

bytrangle Oct 1, 2021 •

edited

erezrokah Oct 1, 2021

bytrangle Oct 1, 2021

erezrokah Oct 1, 2021 •

edited

erezrokah Sep 30, 2021

bytrangle commented Oct 12, 2021

bytrangle commented Oct 27, 2021

erezrokah commented Oct 27, 2021

erezrokah commented Oct 27, 2021

bytrangle commented Apr 13, 2022 •

edited

erezrokah left a comment

erezrokah Apr 13, 2022

bytrangle May 16, 2022 •

edited

erezrokah Apr 13, 2022

erezrokah Apr 13, 2022

erezrokah Apr 13, 2022

bytrangle May 16, 2022 •

edited

stale bot commented Apr 26, 2023

martinjagodic commented Oct 16, 2023

paulRbr commented Feb 28, 2024

martinjagodic commented Feb 28, 2024

bytrangle commented Mar 14, 2024

martinjagodic commented Mar 14, 2024

bytrangle commented Apr 1, 2024

demshy commented Apr 2, 2024

	this.branch = defaultBranch \|\| '';
	this.branch = defaultBranch \|\| 'master';

Fix: Set correct branch when it's not specified in the config #5844

Fix: Set correct branch when it's not specified in the config #5844

Conversation

bytrangle commented Sep 27, 2021

bytrangle commented Sep 29, 2021

erezrokah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bytrangle Oct 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erezrokah Oct 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bytrangle commented Oct 12, 2021

bytrangle commented Oct 27, 2021

erezrokah commented Oct 27, 2021

erezrokah commented Oct 27, 2021

bytrangle commented Apr 13, 2022 • edited

erezrokah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bytrangle May 16, 2022 • edited

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

bytrangle May 16, 2022 • edited

Choose a reason for hiding this comment

stale bot commented Apr 26, 2023

martinjagodic commented Oct 16, 2023

paulRbr commented Feb 28, 2024

martinjagodic commented Feb 28, 2024

bytrangle commented Mar 14, 2024

martinjagodic commented Mar 14, 2024

bytrangle commented Apr 1, 2024

demshy commented Apr 2, 2024

bytrangle Oct 1, 2021 •

edited

erezrokah Oct 1, 2021 •

edited

bytrangle commented Apr 13, 2022 •

edited

bytrangle May 16, 2022 •

edited

bytrangle May 16, 2022 •

edited