decap-cms icon indicating copy to clipboard operation
decap-cms copied to clipboard

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

Open bytrangle opened this issue 4 years ago • 17 comments

fix #5617

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

  1. In the admin/config.yml file, specify a valid Github repo path to push content to.
  2. Run yarn start.
  3. 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:

  • [x] I have read the contribution guidelines.
  • [x] 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 avatar Sep 27 '21 09:09 bytrangle

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

bytrangle avatar Sep 29 '21 14:09 bytrangle

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 avatar Oct 12 '21 09:10 bytrangle

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

bytrangle avatar Oct 27 '21 09:10 bytrangle

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

erezrokah avatar Oct 27 '21 10:10 erezrokah

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

erezrokah avatar Oct 27 '21 10:10 erezrokah

Let me walk through what I've done:

  1. Click on the Action tab of Netlify CMS's repo.
  2. Look for my failed action and click it.
  3. I'm directed to the dedicated page for the action but I didn't see any Rerun Jobs button.

action-page

action-run

bytrangle avatar Oct 27 '21 13:10 bytrangle

Can you try clicking the details button directly? image

That should take you to the specific run

erezrokah avatar Oct 27 '21 14:10 erezrokah

I can, but there's no Rerun Jobs button that I can see. Anyway, I thought it's only possible to rerun the full pipeline, not a single job?

specific-run

Btw, my code changes are ready for review again, if you have time.

bytrangle avatar Oct 28 '21 07:10 bytrangle

Oh dear Erez, please never mind. I can see the Rerun button after accept your invitation. At the time of asking, I didn't have write access yet, so it didn't appear. I should have known it.

bytrangle avatar Oct 28 '21 08:10 bytrangle

@erezrokah I'm so sorry for dropping the ball on this. Do you still need this issue to be worked on? I can get back to it in a few days.

bytrangle avatar Mar 11 '22 18:03 bytrangle

@erezrokah I'm so sorry for dropping the ball on this. Do you still need this issue to be worked on? I can get back to it in a few days.

No worries, I appreciate the work you've done so far. I'd be happy to see this land and merged if you have the time to finalize it.

erezrokah avatar Mar 14 '22 09:03 erezrokah

@erezrokah Thank you for your kind words.

Update on my progress: My changes have led to a bunch of failed unit tests. In my newest commit, I tackled the first failed test I saw named stores and returns user object on success.

What happens under the hood is the test calls an authenticate method. This method calls the endpoint /projects/:id twice:

  • the first time for checking if the authenticated user has write access.
  • the second time is for getting the default branch name.

However, the test only intercepts the request to projects/:id once through the interceptAuth() function.

 function interceptAuth(backend, { userResponse, projectResponse } = {}) {
    const api = mockApi(backend);
    // ...
    api
      .get(expectedRepoUrl)
      .query(true)
      .reply(200, projectResponse || resp.project.success);
  }

As a result, the second API request from my codes is never intercepted by Nock, leading to an error: "Nock: No match for request ... "url": "https://gitlab.com/api/v4/projects/foo%2Fbar"".

I circumvented it by repeating the same response twice.

 function interceptAuth(backend, { userResponse, projectResponse } = {}) {
    // ...
    api
      .get(expectedRepoUrl).times(2)
      .query(true)
      .reply(200, projectResponse || resp.project.success);
  }

Hope that makes sense.

bytrangle avatar Mar 14 '22 14:03 bytrangle

@erezrokah I'm trying to test if the function getDefaultBranchName is called when the config for each backend misses the value for branch field. Would you mind giving me some pointers?

Here's what I'm unsure about where to start:

  • I added getDefaultBranchName function to netlify-cms-lib-util so that each backend can import it, thus avoiding repetition.
  • However, the function that calls getDefaultBranchName is authenticate method defined as a property of each namesake class.

So where should I put the test for checking if getDefaultBranchName is called? In the tests folder for each backend, or in the test folder of netlify-cms-lib-util?

For now, I'm putting the test in netlify-cms-backend-gitlab/src/__tests/gitlab.spec.js and it passed.

// ...
jest.mock('netlify-cms-lib-util', () => ({
  ...jest.requireActual('netlify-cms-lib-util'),
  getDefaultBranchName: jest.fn().mockResolvedValue('master')
}))
// ...
    it('getDefaultBranchName is called when branch field value is empty in the config', async () => {
      const backendName = defaultConfig.backend.name
      backend = resolveBackend(defaultConfig)
      interceptAuth(backend)
      await backend.authenticate(mockCredentials)
      expect(getDefaultBranchName).toHaveBeenCalledTimes(1)
      expect(getDefaultBranchName).toHaveBeenCalledWith({ backend: backendName, repo, token: mockCredentials.token })
    })

What do you think of it?

bytrangle avatar Mar 18 '22 10:03 bytrangle

@erezrokah Would you mind review this PR again since I've made significant changes to it since the last review?

bytrangle avatar Apr 13 '22 09:04 bytrangle

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

bytrangle avatar Apr 13 '22 10:04 bytrangle

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.

stale[bot] avatar Apr 26 '23 09:04 stale[bot]

@bytrangle are you still interested in moving this forward?

martinjagodic avatar Oct 16 '23 09:10 martinjagodic

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.

paulRbr avatar Feb 28 '24 09:02 paulRbr

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

martinjagodic avatar Feb 28 '24 11:02 martinjagodic

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

bytrangle avatar Mar 14 '24 13:03 bytrangle

@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.

martinjagodic avatar Mar 14 '24 13:03 martinjagodic

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

bytrangle avatar Apr 01 '24 10:04 bytrangle

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

demshy avatar Apr 02 '24 07:04 demshy