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

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

Merged
merged 72 commits into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
72 commits
Select commit Hold shift + click to select a range
47ebd16
feat: add helper for fetching default branch from Github
bytrangle Sep 27, 2021
61904c8
feat: add method for setting default branch
bytrangle Sep 27, 2021
c3f1fc0
fix: set default branch after user has authenticated successfully
bytrangle Sep 27, 2021
90c9f9c
fix: format code
bytrangle Sep 27, 2021
43fbe2d
feat: add unit test for getting default branch name
bytrangle Sep 30, 2021
8436443
feat: add helpers for parsing API responses
bytrangle Oct 11, 2021
3edc3ce
feat(lib-util): add helper for constructing request headers
bytrangle Oct 11, 2021
7ccbac6
feat(lib-util): add helper for constructing full URL for API request
bytrangle Oct 11, 2021
e5407c3
feat(lib-util): store base URLs for each backend
bytrangle Oct 11, 2021
3f0e711
feat(lib-util): add type annotation for the request config
bytrangle Oct 11, 2021
a506b5d
feat(lib-util): add helper for handle API request error
bytrangle Oct 11, 2021
cc5f53a
feat(lib-util): add config for making api request
bytrangle Oct 11, 2021
cec8fbb
feat(lib-util): add api request generator
bytrangle Oct 11, 2021
69d124a
feat(lib-util): add helper for getting default branch name
bytrangle Oct 11, 2021
c726ee0
feat(lib-util): export method for getting default branch name
bytrangle Oct 11, 2021
bbafe77
feat(gh-backend): add a boolean property to check if branch is config…
bytrangle Oct 11, 2021
11e187c
feat(gh-backend): set prop `branch` as `master` when it's missing in …
bytrangle Oct 11, 2021
67904b1
feat(gh-backend): set branch name when it's missing in config
bytrangle Oct 11, 2021
37531e1
feat(gitlab-backend): set branch when it's not in the config
bytrangle Oct 12, 2021
b118aea
feat(bitbucket-backend): set branch when it's not specified in config
bytrangle Oct 12, 2021
fa8e03c
feat(lib-util): allow token type to be undefined
bytrangle Oct 12, 2021
a2add4a
fix: format codes
bytrangle Oct 12, 2021
eb9ce4b
Merge branch 'master' into fix/default-branch
bytrangle Mar 11, 2022
8319d4b
feat(github): removed setDefaultBranch function
bytrangle Mar 12, 2022
87350d9
feat(github): remove function for getting default branch
bytrangle Mar 12, 2022
7b0fda0
Merge branch 'master' into fix/default-branch
bytrangle Mar 12, 2022
542a1d9
Merge branch 'fix/default-branch' of https://github.com/bytrangle/net…
bytrangle Mar 12, 2022
3694c75
fix (github): removed GithubRepo object because it was never used
bytrangle Mar 12, 2022
346ac8b
fix (gitlab test): Repeat response for getting project info 2 times
bytrangle Mar 14, 2022
714a57e
fix(gitlab test): add property `default_branch` to project response
bytrangle Mar 14, 2022
f77dba5
fix(gitlab test): reformat codes
bytrangle Mar 14, 2022
9c14584
feat(lib util api): change function name
bytrangle Mar 17, 2022
acd47a8
feat(lib-util api): Change variable name for storing API roots
bytrangle Mar 17, 2022
98541a3
feat(lib-util api): Add varialbe for storing endpoint constants
bytrangle Mar 17, 2022
7dc9dae
feat(lib-util api): Change the returned value for `getDefaultBranchName`
bytrangle Mar 17, 2022
1ecd946
feat(api test): Import Nock module for mocking API requests
bytrangle Mar 17, 2022
efa741f
feat(api test): Add default values for mocking API
bytrangle Mar 17, 2022
7bf6724
feat(api test): Add mock response to getting a single repo
bytrangle Mar 17, 2022
2dfa3f6
feat(api test): Add function for itnercepting request to get single repo
bytrangle Mar 17, 2022
ef664f6
feat(api test): Add test for gettingDefaultBranchName
bytrangle Mar 17, 2022
89a2806
feat(lib-util): reformat codes
bytrangle Mar 17, 2022
54f4314
Merge branch 'master' into fix/default-branch
erezrokah Mar 25, 2022
183a0c3
feat(jest config): add moduleNameMapper for GitHub and BitBucket
bytrangle Apr 11, 2022
02ef43c
feat(lib-util test): return some modules from backend core for testing
bytrangle Apr 11, 2022
5d1855c
feat(lib-util test): add owner login value for Github's repo response
bytrangle Apr 11, 2022
b03d6a8
feat(lib-util test): change access level for Gitlab to 30
bytrangle Apr 11, 2022
13f0863
feat(lib-util test): add mock response for getting a user
bytrangle Apr 11, 2022
1aba0da
feat(lib-util test): add default config for backend field
bytrangle Apr 11, 2022
1f76df9
feat(lib-util test): rewrite function for mocking API
bytrangle Apr 11, 2022
394dc9b
feat(lib-util test): rewrite function for mocking request for repo
bytrangle Apr 11, 2022
0fc96ab
test(lib-util): rewrite test for the function getDefaultBranchName
bytrangle Apr 11, 2022
6eb2ac3
test(lib-util): add function for resolving backend
bytrangle Apr 11, 2022
6bfcf09
test(lib-util): import 'set' module from Lodash
bytrangle Apr 11, 2022
53f143a
test(lib-util): add helper for constructing url path for each backend
bytrangle Apr 11, 2022
38f73a4
test(lib-util): add function for intercepting API request to authenti…
bytrangle Apr 11, 2022
af9cd9b
test(lib-util): import each backend module
bytrangle Apr 11, 2022
a5da48a
Merge branch 'master' into fix/default-branch
erezrokah Apr 12, 2022
14ddaa7
test(lib-util): add tests that check each backend calls getDefaultBra…
bytrangle Apr 13, 2022
02311f4
Merge branch 'fix/default-branch' of https://github.com/bytrangle/net…
bytrangle Apr 13, 2022
1789e6f
style: format files
erezrokah Apr 13, 2022
34375f4
fix: query branch name before setting Github API service
bytrangle May 17, 2022
7f6de72
fix: reformat implementation module of Github backend
bytrangle May 17, 2022
ec0ff89
fix: remove importing of getDefaultBranchName from lib
bytrangle May 17, 2022
48e7175
fix: removed test for getDefaultBranchName in lib packages
bytrangle May 19, 2022
676631f
fix: removed unused vars in api test for lib package
bytrangle May 19, 2022
614a5b0
feat: retrieve default branch before creating Bitbucket AI instance
bytrangle May 26, 2022
20caf5f
fix: reformat codes in Bitbucket implementation module
bytrangle May 26, 2022
8db27d6
fix: Resolve conflicts from changing cms
bytrangle Mar 18, 2024
ad161df
Merge branch 'master' into fix/default-branch
martinjagodic Mar 18, 2024
d4e4fe3
Merge branch 'main' into fix/default-branch
demshy Apr 2, 2024
c6b05f8
fix: add missing import
demshy Apr 2, 2024
5827471
Merge branch 'main' into fix/default-branch
demshy Apr 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions packages/netlify-cms-backend-github/src/API.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ type GitHubCompareCommit = Octokit.ReposCompareCommitsResponseCommitsItem;
type GitHubAuthor = Octokit.GitCreateCommitResponseAuthor;
type GitHubCommitter = Octokit.GitCreateCommitResponseCommitter;
type GitHubPull = Octokit.PullsListResponseItem;
type GithubRepo = Octokit.ReposGetResponse;

export const API_NAME = 'GitHub';

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

async newGetDefaultBranch() {
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const result: GithubRepo = await this.request(`${this.originRepoURL}`);
return result.default_branch;
} catch (e) {
console.error('Problem fetching repo data from Github');
return null;
}
}

async backupBranch(branchName: string) {
try {
const existingBranch = await this.getBranch(branchName);
Expand Down
15 changes: 15 additions & 0 deletions packages/netlify-cms-backend-github/src/__tests__/API.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,21 @@ describe('github API', () => {
});
});

describe('newGetDefaultBranch', () => {
it('should return a non-empty value for the default branch', async () => {
const defaultBranch = 'main';
const repo = `owner/my-repo`;
const api = new API({ branch: defaultBranch, repo });
const responses = {
'/repos/owner/my-repo': () => ({ default_branch: defaultBranch }),
};
mockAPI(api, responses);

await expect(api.newGetDefaultBranch()).resolves.toBe(defaultBranch);
expect(api.request).toHaveBeenCalledTimes(1);
expect(api.request.mock.calls[0]).toEqual([`/repos/${repo}`]);
});
});
describe('migratePullRequest', () => {
it('should migrate to pull request labels when no version', async () => {
const api = new API({ branch: 'master', repo: 'owner/repo' });
Expand Down
23 changes: 22 additions & 1 deletion packages/netlify-cms-backend-github/src/implementation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ export default class GitHub implements Implementation {
this.repo = this.originRepo = config.backend.repo || '';
}
this.alwaysForkEnabled = config.backend.always_fork || false;
this.branch = config.backend.branch?.trim() || 'master';
this.branch = config.backend.branch?.trim() || '';
this.apiRoot = config.backend.api_root || 'https://api.github.com';
this.token = '';
this.squashMerges = config.backend.squash_merges || false;
Expand Down Expand Up @@ -180,6 +180,20 @@ export default class GitHub implements Implementation {
: this.authenticate(user);
}

async setDefaultBranch() {
try {
const masterBranch = await this.api!.getBranch('master');
if (!masterBranch) {
// Get default branch of the repo
const defaultBranch = await this.api!.newGetDefaultBranch();
this.branch = defaultBranch || '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@bytrangle bytrangle Oct 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

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

  2. 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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@erezrokah erezrokah Oct 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
this.branch = 'master';
}
} catch (e) {
console.warn(e);
bytrangle marked this conversation as resolved.
Show resolved Hide resolved
}
}
async pollUntilForkExists({ repo, token }: { repo: string; token: string }) {
const pollDelay = 250; // milliseconds
let repoExists = false;
Expand Down Expand Up @@ -331,6 +345,13 @@ export default class GitHub implements Implementation {
if (!isCollab) {
throw new Error('Your GitHub user account does not have access to this repo.');
}
// In the constructor, `this.branch` is set by reading the config file
// If it is an empty string at this point,
// it means there is no `branch` property set in the config

if (this.branch === '') {
await this.setDefaultBranch();
}

// Authorized user
return { ...user, token: state.token as string, useOpenAuthoring: this.useOpenAuthoring };
Expand Down