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

New BitBucket implementation #1504

Merged
merged 2 commits into from
Jul 24, 2018
Merged

New BitBucket implementation #1504

merged 2 commits into from
Jul 24, 2018

Conversation

Benaiah
Copy link
Contributor

@Benaiah Benaiah commented Jul 20, 2018

Closes #234.

- Summary

Adds support for BitBucket as a backend, both with and without git-gateway. (git-gateway support depends on the merge and deployment of netlify/git-gateway#20). Includes token refresh logic, so users won't have to log in every couple hours to keep using the CMS.

- Test plan

Tested manually.

- Description for the changelog

Add BitBucket backend

- A picture of a cute animal (not mandatory but encouraged)

image

@verythorough
Copy link
Contributor

verythorough commented Jul 20, 2018

Deploy preview for netlify-cms-www ready!

Built with commit 63806f3

https://deploy-preview-1504--netlify-cms-www.netlify.com

@verythorough
Copy link
Contributor

verythorough commented Jul 20, 2018

Deploy preview for cms-demo ready!

Built with commit 63806f3

https://deploy-preview-1504--cms-demo.netlify.com

Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

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

Great work @Benaiah! Found one issue in local testing beyond the handful of code review comments: if you open an entry and then go back to the collections screen, pagination no longer works.

We're very close, excited to get this merged.

sem.leave();
}).catch((error = true) => {
sem.leave();
console.error(`failed to load file from GitLab: ${ file.path }`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This was a confusing message to see in my console lol

authenticate(user) {
this.tokenPromise = user.jwt.bind(user);
// this.tokenPromise = user.jwt.bind(user);
this.tokenPromise = () => Promise.resolve("eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VyX21ldGFkYXRhIjp7fSwiYXBwX21ldGFkYXRhIjp7InJvbGVzIjpbImFkbWluIl19fQ.5W7AGwZ8h5Y1YfHXhaMO1ysviOrKJhcCHGd4SSZ316E");
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to switch this one back.

}
} else if (bitbucket_enabled) {
this.backendType = "bitbucket";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: fix indentation

@@ -98,6 +109,14 @@ export default class GitGateway {
} else if (this.backendType === "gitlab") {
this.api = new GitLabAPI(apiConfig);
this.backend = new GitLabBackend(this.config, { proxied: true, API: this.api });
} else if (this.backendType === "bitbucket") {
this.api = new BitBucketAPI({
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation

hasWriteAccess: async () => true,
});
console.log(this.api);
this.backend = new BitBucketBackend(this.config, { proxied: true, API: this.api });
Copy link
Contributor

@erquhart erquhart Jul 20, 2018

Choose a reason for hiding this comment

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

indentation lol. obviously due to your stubborn use of emacs :trollface:. also we need prettier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 to getting prettier in

const responseFormatters = fromJS({
json: async res => {
const contentType = res.headers.get("Content-Type");
if (!contentType.startsWith("application/json") && contentType.startsWith("text/json")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be switched to just check for startsWith('text/json').

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should actually be !contentType.startsWith("application/json") && !contentType.startsWith("text/json"), but the second ! was missing. Parsing a Content-Type of "text/json" as JSON shouldn't produce an error.

return flow([
unsentRequest.withMethod("POST"),
unsentRequest.withBody(body),
this.request,
Copy link
Contributor

Choose a reason for hiding this comment

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

Love this pattern for building requests, awesome stuff.

export const parseResponse = async (res, { expectingOk = true, format = "text" } = {}) => {
if (expectingOk && !res.ok) {
throw new Error(`Expected an ok response, but received an error status: ${ res.status }.`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noting that this triggers UI errors in situations that don't currently show errors in other backends. For example, a collection in the configuration whose folder does not exist in the repo shows this error, while failing silently in other backends.

@Benaiah Benaiah force-pushed the new-bitbucket-implementation branch from 9250265 to 63806f3 Compare July 23, 2018 19:23
@erquhart erquhart merged commit b690109 into master Jul 24, 2018
@erquhart erquhart deleted the new-bitbucket-implementation branch July 24, 2018 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Netlify CMS on Bitbucket
3 participants