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 60 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
3 changes: 3 additions & 0 deletions jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ module.exports = {
'netlify-cms-lib-util': '<rootDir>/packages/netlify-cms-lib-util/src/index.ts',
'netlify-cms-ui-default': '<rootDir>/packages/netlify-cms-ui-default/src/index.js',
'netlify-cms-backend-github': '<rootDir>/packages/netlify-cms-backend-github/src/index.ts',
'netlify-cms-backend-gitlab': '<rootDir>/packages/netlify-cms-backend-gitlab/src/index.ts',
'netlify-cms-backend-bitbucket':
'<rootDir>/packages/netlify-cms-backend-bitbucket/src/index.ts',
'netlify-cms-lib-widgets': '<rootDir>/packages/netlify-cms-lib-widgets/src/index.ts',
'netlify-cms-widget-object': '<rootDir>/packages/netlify-cms-widget-object/src/index.js',
'\\.(css|less)$': '<rootDir>/__mocks__/styleMock.js',
Expand Down
14 changes: 13 additions & 1 deletion packages/netlify-cms-backend-bitbucket/src/implementation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
allEntriesByFolder,
AccessTokenError,
branchFromContentKey,
getDefaultBranchName,
} from 'netlify-cms-lib-util';
import { NetlifyAuthenticator } from 'netlify-cms-lib-auth';

Expand Down Expand Up @@ -72,6 +73,7 @@ export default class BitbucketBackend implements Implementation {
initialWorkflowStatus: string;
};
repo: string;
isBranchConfigured: boolean;
branch: string;
apiRoot: string;
baseUrl: string;
Expand Down Expand Up @@ -111,6 +113,7 @@ export default class BitbucketBackend implements Implementation {

this.repo = config.backend.repo || '';
this.branch = config.backend.branch || 'master';
this.isBranchConfigured = config.backend.branch ? true : false;
this.apiRoot = config.backend.api_root || 'https://api.bitbucket.org/2.0';
this.baseUrl = config.base_url || '';
this.siteId = config.site_id || '';
Expand Down Expand Up @@ -216,7 +219,16 @@ export default class BitbucketBackend implements Implementation {
if (!isCollab) {
throw new Error('Your BitBucket user account does not have access to this repo.');
}

if (!this.isBranchConfigured) {
const defaultBranchName = await getDefaultBranchName({
backend: 'bitbucket',
repo: this.repo,
token: this.token,
});
if (defaultBranchName) {
this.branch = defaultBranchName;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@bytrangle bytrangle May 16, 2022

Choose a reason for hiding this comment

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

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?

}
}
const user = await this.api.user();

// Authorized user
Expand Down
17 changes: 16 additions & 1 deletion packages/netlify-cms-backend-github/src/implementation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
contentKeyFromBranch,
unsentRequest,
branchFromContentKey,
getDefaultBranchName,
} from 'netlify-cms-lib-util';

import AuthenticationPage from './AuthenticationPage';
Expand Down Expand Up @@ -69,6 +70,7 @@ export default class GitHub implements Implementation {
initialWorkflowStatus: string;
};
originRepo: string;
isBranchConfigured: boolean;
repo?: string;
openAuthoringEnabled: boolean;
useOpenAuthoring?: boolean;
Expand Down Expand Up @@ -103,7 +105,7 @@ export default class GitHub implements Implementation {
}

this.api = this.options.API || null;

this.isBranchConfigured = config.backend.branch ? true : false;
this.openAuthoringEnabled = config.backend.open_authoring || false;
if (this.openAuthoringEnabled) {
if (!this.options.useWorkflow) {
Expand Down Expand Up @@ -332,6 +334,19 @@ export default class GitHub implements Implementation {
throw new Error('Your GitHub user account does not have access to this repo.');
}

// Only set default branch name when the `branch` property is missing
// in the config file
if (!this.isBranchConfigured) {
const defaultBranchName = await getDefaultBranchName({
backend: 'github',
repo: this.originRepo,
token: this.token,
});
if (defaultBranchName) {
this.branch = defaultBranchName;
Copy link
Contributor

Choose a reason for hiding this comment

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

See my previous comment about setting this.branch too late

}
}

// Authorized user
return { ...user, token: state.token as string, useOpenAuthoring: this.useOpenAuthoring };
}
Expand Down
10 changes: 10 additions & 0 deletions packages/netlify-cms-backend-gitlab/src/__tests__/gitlab.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ const resp = {
access_level: 30,
},
},
default_branch: 'main',
},
readOnly: {
permissions: {
Expand Down Expand Up @@ -194,7 +195,16 @@ describe('gitlab backend', () => {
.reply(200, userResponse || resp.user.success);

api
// The `authenticate` method of the API class from netlify-cms-backend-gitlab
// calls the same endpoint twice for gettng a single project.
// First time through `this.api.hasWriteAccess()
// Second time through the method `getDefaultBranchName` from lib-util
// As a result, we need to repeat the same response twice.
// Otherwise, we'll get an error: "No match for request to
// https://gitlab.com/api/v4"

.get(expectedRepoUrl)
.times(2)
.query(true)
.reply(200, projectResponse || resp.project.success);
}
Expand Down
13 changes: 13 additions & 0 deletions packages/netlify-cms-backend-gitlab/src/implementation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
allEntriesByFolder,
filterByExtension,
branchFromContentKey,
getDefaultBranchName,
} from 'netlify-cms-lib-util';

import AuthenticationPage from './AuthenticationPage';
Expand Down Expand Up @@ -53,6 +54,7 @@ export default class GitLab implements Implementation {
initialWorkflowStatus: string;
};
repo: string;
isBranchConfigured: boolean;
branch: string;
apiRoot: string;
token: string | null;
Expand Down Expand Up @@ -84,6 +86,7 @@ export default class GitLab implements Implementation {

this.repo = config.backend.repo || '';
this.branch = config.backend.branch || 'master';
this.isBranchConfigured = config.backend.branch ? true : false;
this.apiRoot = config.backend.api_root || 'https://gitlab.com/api/v4';
this.token = '';
this.squashMerges = config.backend.squash_merges || false;
Expand Down Expand Up @@ -150,6 +153,16 @@ export default class GitLab implements Implementation {
throw new Error('Your GitLab user account does not have access to this repo.');
}

if (!this.isBranchConfigured) {
const defaultBranchName = await getDefaultBranchName({
backend: 'gitlab',
repo: this.repo,
token: this.token,
});
if (defaultBranchName) {
this.branch = defaultBranchName;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as other backends

}
}
// Authorized user
return { ...user, login: user.username, token: state.token as string };
}
Expand Down
154 changes: 154 additions & 0 deletions packages/netlify-cms-lib-util/src/API.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,26 @@ class RateLimitError extends Error {
}
}

async function parseJsonResponse(response: Response) {
const json = await response.json();
if (!response.ok) {
return Promise.reject(json);
}
return json;
}

export function parseResponse(response: Response) {
const contentType = response.headers.get('Content-Type');
if (contentType && contentType.match(/json/)) {
return parseJsonResponse(response);
}
const textPromise = response.text().then(text => {
if (!response.ok) return Promise.reject(text);
return text;
});
return textPromise;
}

export async function requestWithBackoff(
api: API,
req: ApiRequest,
Expand Down Expand Up @@ -96,6 +116,140 @@ export async function requestWithBackoff(
}
}

// Options is an object which contains all the standard network request properties
// for modifying HTTP requests and may contains `params` property

type Param = string | number;

type ParamObject = Record<string, Param>;

type HeaderObj = Record<string, string>;

type HeaderConfig = {
headers?: HeaderObj;
token?: string | undefined;
};

type Backend = 'github' | 'gitlab' | 'bitbucket';

// RequestConfig contains all the standard properties of a Request object and
// several custom properties:
// - "headers" property is an object whose properties and values are string types
// - `token` property to allow passing tokens for users using a private repo.
// - `params` property for customizing response
// - `backend`(compulsory) to specify which backend to be used: Github, Gitlab etc.

type RequestConfig = Omit<RequestInit, 'headers'> &
HeaderConfig & {
backend: Backend;
params?: ParamObject;
};

export const apiRoots = {
github: 'https://api.github.com',
gitlab: 'https://gitlab.com/api/v4',
bitbucket: 'https://api.bitbucket.org/2.0',
};

export const endpointConstants = {
singleRepo: {
bitbucket: '/repositories',
github: '/repos',
gitlab: '/projects',
},
};

const api = {
buildRequest(req: ApiRequest) {
return req;
},
};

function constructUrlWithParams(url: string, params?: ParamObject) {
if (params) {
const paramList = [];
for (const key in params) {
paramList.push(`${key}=${encodeURIComponent(params[key])}`);
}
if (paramList.length) {
url += `?${paramList.join('&')}`;
}
}
return url;
}

async function constructRequestHeaders(headerConfig: HeaderConfig) {
const { token, headers } = headerConfig;
const baseHeaders: HeaderObj = { 'Content-Type': 'application/json; charset=utf-8', ...headers };
if (token) {
baseHeaders['Authorization'] = `token ${token}`;
Copy link

@floscher floscher Jun 28, 2024

Choose a reason for hiding this comment

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

Hi @bytrangle, do you by any chance remember, why this is using token instead of Bearer here?
This seems to be causing an issue with Gitlab logins, since this PR was merged in April: #7172 (comment)

I'm not sure about Github/Bitbucket, but it seems at least for Gitlab this line should be like this:

Suggested change
baseHeaders['Authorization'] = `token ${token}`;
baseHeaders['Authorization'] = `Bearer ${token}`;

For reference: This line was originally introduced in 3edc3ce, this comment is on a2add4a, which was later squashed to c91a70f

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't remember. This corresponds to Gitlab docs 13.0, which is no longer available. Please feel free to make a new PR to correct it.

Choose a reason for hiding this comment

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

Thanks anyway, I now created #7242 for this change.

Copy link

@b-xb b-xb Jul 1, 2024

Choose a reason for hiding this comment

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

Nice work @floscher

I cloned the decapCMS repo locally yesterday to double check the fix.

After adding the fix and rebuilding decapCMS, I no longer got the previous error message.

However, I then still got an "API_ERROR: Branch not found" message, implying that the code this patch was meant to fix doesn't do it's job on gitlab anyway..

Only after hard-coding "branch: main" into my config.yml, was I then able to get on with things.

}
return Promise.resolve(baseHeaders);
}

function handleRequestError(error: FetchError, responseStatus: number, backend: Backend) {
throw new APIError(error.message, responseStatus, backend);
}

export async function apiRequest(
path: string,
config: RequestConfig,
parser = (response: Response) => parseResponse(response),
) {
const { token, backend, ...props } = config;
const options = { cache: 'no-cache', ...props };
const headers = await constructRequestHeaders({ headers: options.headers || {}, token });
const baseUrl = apiRoots[backend];
const url = constructUrlWithParams(`${baseUrl}${path}`, options.params);
let responseStatus = 500;
try {
const req = unsentRequest.fromFetchArguments(url, {
...options,
headers,
}) as unknown as ApiRequest;
const response = await requestWithBackoff(api, req);
responseStatus = response.status;
const parsedResponse = await parser(response);
return parsedResponse;
} catch (error) {
return handleRequestError(error, responseStatus, backend);
}
}

export async function getDefaultBranchName(configs: {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@bytrangle bytrangle May 16, 2022

Choose a reason for hiding this comment

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

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.

backend: Backend;
repo: string;
token?: string;
}) {
let apiPath;
const { token, backend, repo } = configs;
switch (backend) {
case 'gitlab': {
apiPath = `/projects/${encodeURIComponent(repo)}`;
break;
}
case 'bitbucket': {
apiPath = `/repositories/${repo}`;
break;
}
default: {
apiPath = `/repos/${repo}`;
}
}
const repoInfo = await apiRequest(apiPath, { token, backend });
let defaultBranchName;
if (backend === 'bitbucket') {
const {
mainbranch: { name },
} = repoInfo;
defaultBranchName = name;
} else {
const { default_branch } = repoInfo;
defaultBranchName = default_branch;
}
return defaultBranchName;
}

export async function readFile(
id: string | null | undefined,
fetchContent: () => Promise<string | Blob>,
Expand Down
Loading