Fix: Set correct branch when it's not specified in the config #5844
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -49,15 +49,15 @@ async function parseJsonResponse(response: Response) { | |||||
| } | ||||||
|
|
||||||
| 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( | ||||||
|
|
@@ -119,18 +119,18 @@ 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: | ||||||
|
|
@@ -139,100 +139,107 @@ type Backend = "github" | "gitlab" | "bitbucket" | |||||
| // - `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 rootApi = { | ||||||
| github: 'https://api.github.com', | ||||||
| gitlab: 'https://gitlab.com/api/v4', | ||||||
| bitbucket: 'https://api.bitbucket.org/2.0', | ||||||
| }; | ||||||
|
|
||||||
| const api = { | ||||||
| buildRequest(req: ApiRequest) { | ||||||
| return req; | ||||||
| }, | ||||||
| }; | ||||||
|
|
||||||
| function constructUrl(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. 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. 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. Thanks anyway, I now created #7242 for this change. There was a problem hiding this comment. 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 = rootApi[backend]; | ||||||
| const url = constructUrl(`${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. 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. 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 || null; | ||||||
| } | ||||||
|
|
||||||
| export async function readFile( | ||||||
|
|
||||||
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.branchhere is too late? We already passedthis.branchto 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
restoreUserseems to be the earliest function called in theimplementationmodule, followed byauthenticate. Should I callgetDefaultBranchNamebefore setting up API inauthenticate, or do you recommend another location?