-
-
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
(WIP) Add GitLab Backend #517
Conversation
c28b852
to
4cd83f9
Compare
9ea2caf
to
c3ff76d
Compare
@zanedev brought up the fact that the current implementation does not work with OAuth |
99d0ad9
to
1b8f2e8
Compare
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.
Quick review of request
and fileExists
in the GitLab API.
src/backends/gitlab/API.js
Outdated
} | ||
return Promise.all([response, response.text()]); | ||
}) | ||
.catch(err => [err, null]) |
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.
This should be .catch(err => Promise.reject([err, null))
so the other .catch
block catches and destructures it properly. This is my bad - it's the same way in my GH API refactor PR.
src/backends/gitlab/API.js
Outdated
.catch(([errorValue, response]) => { | ||
const errorMessageProp = (errorValue && errorValue.message) ? errorValue.message : null; | ||
const message = errorMessageProp || (isString(errorValue) ? errorValue : ""); | ||
throw new APIError(message, response && response.status, 'GitHub', { response, errorValue }); |
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.
Change 'GitHub'
to 'GitLab'
src/backends/gitlab/API.js
Outdated
}).then(() => true).catch((err) => { | ||
// TODO: 404 can mean either the file does not exist, or if an API | ||
// endpoint doesn't exist. Is there a better way to check for this? | ||
if (err.status === 404) {return false;} else {throw err;} |
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.
You can change this whole block to .catch(err => (err.status === 404 ? false : Promise.reject(err)))
. I think the 404 check is fine, unless GitLab has something more specific we can check (you can access the whole response
object as err.meta
).
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.
The problem is that with a HEAD request, we don't actually get the content of the error message back, so we can't tell the difference.
Is this still in progress? I'm interested in working on this (in fact have already started before I saw this.) |
@cbarrett @theetrain Yes, this is still in progress. Right now we have mostly been working on bugfixes and critical features for 1.0 (see https://github.com/netlify/netlify-cms/wiki/2017.09.13-Planning-Session for an update), but as soon as we get those done, we'll get back to this. If you need to use it right now you should be able to rebase it onto |
Would love to know when this is in. |
@tech4him1 is the Netlify auth api currently supported/enabled for GitLab? I've merged your gitlab branch with 0.7.6 and got it running but I'm facing a 'No Auth Provider Found' when the pop-up window attempts to access Also, are there any plans for direct auth since GitLab supports implicit grant? |
@nnyath The Netlify Auth API does support GitLab, but you will still need to set it up like you do with the GitHub backend: https://www.netlify.com/docs/authentication-providers/#using-an-authentication-provider. That's a very good point about implicit auth, and we've actually been discussing this within the team lately. @Benaiah @erquhart Is there any reason not to allow implicit auth with the backends that support it? |
Development has moved to #1343, which refactors the CMS and the GitLab backend to use cursors for sophisticated pagination support. While development and code review will be done there, I will continue to update this PR as we hit milestones, since so many people are subscribed here for updates. |
#1343 is now ready for review! |
them. This is not optimal, but it is how the GitHub implementation works and should be fixed later.
This reverts commit f789472.
`hasWriteAccess` was not working in the case where permissions were granted on a single group-owned repo, instead of the entire group.
Rebased and added a commit for single direction pagination support. |
- Summary
Add support for GitLab as a backend. This version does not support the editorial workflow. Fixes #57. This PR copies the code from #508, so those fixes can be used for this backend as well. It is also (re)based on #560 -- once that one is merged this will be rebased.
- Todos
request
andfileExists
functions more maintainable -- see TODOs in code.api_root
should work.- Test plan
Manually tested with the
example
project and a "real" project.- Description for the changelog
Add support for GitLab as a backend (simple workflow only).