-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Changes from 60 commits
47ebd16
61904c8
c3f1fc0
90c9f9c
43fbe2d
8436443
3edc3ce
7ccbac6
e5407c3
3f0e711
a506b5d
cc5f53a
cec8fbb
69d124a
c726ee0
bbafe77
11e187c
67904b1
37531e1
b118aea
fa8e03c
a2add4a
eb9ce4b
8319d4b
87350d9
7b0fda0
542a1d9
3694c75
346ac8b
714a57e
f77dba5
9c14584
acd47a8
98541a3
7dc9dae
1ecd946
efa741f
7bf6724
2dfa3f6
ef664f6
89a2806
54f4314
183a0c3
02ef43c
5d1855c
b03d6a8
13f0863
1aba0da
1f76df9
394dc9b
0fc96ab
6eb2ac3
6bfcf09
53f143a
38f73a4
af9cd9b
a5da48a
14ddaa7
02311f4
1789e6f
34375f4
7f6de72
ec0ff89
48e7175
676631f
614a5b0
20caf5f
8db27d6
ad161df
d4e4fe3
c6b05f8
5827471
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ import { | |
contentKeyFromBranch, | ||
unsentRequest, | ||
branchFromContentKey, | ||
getDefaultBranchName, | ||
} from 'netlify-cms-lib-util'; | ||
|
||
import AuthenticationPage from './AuthenticationPage'; | ||
|
@@ -69,6 +70,7 @@ export default class GitHub implements Implementation { | |
initialWorkflowStatus: string; | ||
}; | ||
originRepo: string; | ||
isBranchConfigured: boolean; | ||
repo?: string; | ||
openAuthoringEnabled: boolean; | ||
useOpenAuthoring?: boolean; | ||
|
@@ -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) { | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my previous comment about setting |
||
} | ||
} | ||
|
||
// Authorized user | ||
return { ...user, token: state.token as string, useOpenAuthoring: this.useOpenAuthoring }; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ import { | |
allEntriesByFolder, | ||
filterByExtension, | ||
branchFromContentKey, | ||
getDefaultBranchName, | ||
} from 'netlify-cms-lib-util'; | ||
|
||
import AuthenticationPage from './AuthenticationPage'; | ||
|
@@ -53,6 +54,7 @@ export default class GitLab implements Implementation { | |
initialWorkflowStatus: string; | ||
}; | ||
repo: string; | ||
isBranchConfigured: boolean; | ||
branch: string; | ||
apiRoot: string; | ||
token: string | null; | ||
|
@@ -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; | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 }; | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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, | ||||||
|
@@ -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}`; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @bytrangle, do you by any chance remember, why this is using I'm not sure about Github/Bitbucket, but it seems at least for Gitlab this line should be like this:
Suggested change
For reference: This line was originally introduced in 3edc3ce, this comment is on a2add4a, which was later squashed to c91a70f There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks anyway, I now created #7242 for this change. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about having a 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
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>, | ||||||
|
There was a problem hiding this comment.
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 passedthis.branch
to the API ctor a few lines back https://github.com/netlify/netlify-cms/pull/5844/files#diff-09a7e7be36a8f9bbf96a0407a90bee6df174fef8477a6d68de3472067a884643R199There was a problem hiding this comment.
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 theimplementation
module, followed byauthenticate
. Should I callgetDefaultBranchName
before setting up API inauthenticate
, or do you recommend another location?