-
-
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) Bitbucket integration #525
Conversation
return this.api.deleteFile(path, commitMessage, options); | ||
} | ||
|
||
unpublishedEntries() { |
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.
Should these functions be removed since the API
does not support editorial workflow?
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.
Ah good catch I missed those, will remove those and push again.
@zanedev I'm not too familiar with the BitBucket API -- How does your implementation check if the user has permission to access the repo (like |
@tech4him1 actually I spent quite a while trying to work out isCollaborator but couldn't figure out a way to do it with the bitbucket api. I found nowhere to check scopes/permissions on a repo like the github implementation. I thought at a minimum then we could list the users projects then querying that but it's a paginated list of all projects, it would take many queries to go through and I wasn't even sure if that meant they had the correct permissions. Anyway I figure we can come back to this when we figure out or they provide how to do it. |
@zanedev Does this work? You still have to implement pagination, so I don't know if its feasible, but the BB API has permission checking. isCollaborator(user) {
return this.request(`/repositories/${ user.username }`, {
params: { role: "contributor" },
}).then((r) => {
const repos = r.values;
let contributor = false;
for (const repo of repos) {
// You may need to use `${ user.username }/${ repo.name }` here, I haven't tested it yet.
if (repo.name === this.repo) contributor = true;
}
return contributor;
}).catch(/*TODO*/)
} https://developer.atlassian.com/bitbucket/api/2/reference/resource/repositories/%7Busername%7D |
@tech4him1 It does get a list of repos for the user but it's paginated by 10 per page, I personally have over 40, so it would need to keep requesting and paging. If you want me to add logic to to keep requesting until it reaches the end of the list I can do that. Just thought there had to be a better way to grab it directly rather than having to page it. |
const uploadPromises = []; | ||
const files = mediaFiles.concat(entry); | ||
|
||
files.forEach((file) => { |
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.
@zanedev This has been discussed before, and there are obviously pros and cons to both sides, but I wonder if it would be best to upload all the media files first, then upload the entry. That way, if there is a problem with one of the media files, the entry would not be broken. But that is an opinion, some people may prefer to have the content saved first. Anyone else?
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.
I see no problem with doing media files first. I feel like all of these backend implementations need to be refactored to have some shared code to avoid code/logic duplication however.. Maybe it's best to do it all at once for all backends so they follow the same logic and we don't further spaghetti weave it.
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.
@tortilaman and I were actually already talking about combining the backends: https://gitter.im/netlify/NetlifyCMS?at=5989e0e91c8697534a9c1a7d, as I totally think that is an awesome idea. He was going to make an issue for it, but I'm not sure if he got around to that yet.
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.
Yeah I think someone would have to own it across the board to do that it would be a big effort requiring ongoing maintenance and way more testing etc. Not sure if it's worth it yet. But some sort of shared interface would be a good start at least. Typescript might be a good fit for something this delicate.
A basic setup is working for me! |
isCollaborator is implemented and a graceful way to handle backends with unsupported workflow. Let me know if you have other ideas about unsupported workflow, wasn't sure the best way. |
src/backends/bitbucket/API.js
Outdated
return this.request("/user"); | ||
} | ||
|
||
isCollaborator(user) { |
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.
isCollaborator
was changed to hasWriteAccess
in #543.
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.
Ok will update and push again
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.
It was kind of a last minute thing, thanks!
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.
np renamed and pushed, looking at token refresh stuff now
src/backends/backend.js
Outdated
@@ -277,6 +278,13 @@ class Backend { | |||
entry.data[filterRule.get('field')] === filterRule.get('value') | |||
)); | |||
} | |||
|
|||
supportsWorkflow() { | |||
if(typeof this.implementation.supportsWorkflow === "function") |
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.
I wonder if it would be better to just add this function to all the backends instead of this?
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.
Yeah not sure just thought it was more self contained this way at the time but since there are a few backends that don't support workflow now (unfortunately) then it makes sense to add it to all of them. btw, I hope that we can implement workflow for bitbucket and gitlab sooner than later, I think we should create separate issues so they don't fall through the cracks.
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.
Yeah, I think we need to figure out which way is best so that we can keep our PR's the same.
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.
I prefer this approach, but it takes us halfway to what we really need, which is some sort of capabilities record for each backend, of which editorial workflow would be one. For now, I think @tech4him1's approach is ideal in its simplicity.
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.
@zanedev The code that I had just checks to see if editorial workflow is enabled, the user doesn't have to have any other config value.
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.
Well, it is an actual config.yml
value, but it is just the one that is used to enable editorial workflow anyway (publish_mode: editorial_workflow
). Do you think there is a better way @zanedev?
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.
yep I was mistaken in my original comment.. already pushed a version that matches yours.
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.
OK, great. I think we should move to something more like what you had down the road, though, it just seems a little complicated for integrating right now.
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.
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.
thanks i've been meaning to do that..
@tech4him1 @tech4him1 you guys rock. Is there anything I can help with? |
Is this close to being ready to merge in? If I can help at all I'm willing to do so. I'd like to use Netlify CMS and not have to move our repo. :) |
Hi @scher200, @kristyjy - we definitely want to get this in, it just slowed up because focus has been on prepping 1.0, which is slated for early December. We've also discussed having a beta release line so that work like this can get out in the wild for those that want it before it's ready to hit master. But yes, by all means come pitch in! There are some TODO items in the summary up top that I'll re-paste here:
I'm not up to speed on the refresh token issue, but it basically means the CMS stops working when your token expires, and the user has to know to log out and log back in when this occurs. I believe @biilmann had some ideas on how we might get around this, but it's been a while. I'm not really sure what |
Hey, what's the status of this PR? Do you need any help? |
@mikestopcontinues sorry men, I stopped bothering about this, and switched git provider. |
@mikestopcontinues @scher200 Now that 1.0 has been released, this will be one of our primary focuses. |
@tech4him1, awesome! Thanks! |
Yeah the only major blocker here is token refresh. @mikestopcontinues If you want to use it now there is a sample auth server repo linked in the description but you'd need to modify it to work with your bitbucket settings. And the PR is a bit out of date so you'd probably need to merge in master/1.0 code to stay up to date. |
Hi, is the merge working out? |
Hi, me too. I am also willing to help with anything :) |
@tech4him1 sounds cool unfortunately I'm swamped for the next month or two.. I'll try to to get some time to at least get this PR up to date with master but there will still be the refresh token issue so not sure it's worth doing anything until that is provided on the netlify side somehow.. |
b413033
to
7b4ed27
Compare
Deploy preview for netlify-cms-www ready! Built with commit 0dd1495 |
1476eac
to
0dd1495
Compare
Update: rebased and updated for 1.x compatibility. |
Deploy preview for cms-demo ready! Built with commit 0dd1495 |
I made some updates to Bitbucket integration API and implementation (media and hasWriteAccess), where should I post my code please? I never got it how to update foreign pull-request, so if somebody can help me with that :) |
@george-oakling You can make a new PR, and set the base branch to |
Is there any timeline to get this released? |
@Benaiah you mentioned Bitbucket and GitLab support coming once we have the minimal required improvements in place, can you give a rough estimate? |
It's well past time for an update on the progress of this and the GitLab PR - apologies for the radio silence. With that said, I've laid out the current plan for this PR below. Before mergeI'll check these off as they're addressed by PRs, and link to any PRs implementing them. Most of these steps are shared with those of the GitLab backend, since they are modifications of code that's used by both.
Rough timelineThe dates are approximate, but this is a rough idea of when I think certain updates will happen:
The work on the new backend design will be concurrent with this, but pre-merge work is the priority. After merge
|
UpdateThis is still underway, but it's now planned to merge the GitLab backend first and then update and merge this PR. Quoting from my comment on the GitLab PR:
|
I still want to use NetlifyCMS with Gatsby and BitBucket. I will be happy to help with some testing. |
I am also interested in using NetlifyCMS with Bitbucket. I can be beta tester of the this PR 👍 |
To all watchers: keep an eye on the current GitLab PR, as the underlying work is prerequisite to Bitbucket support. |
Yes, I'd also like to test this with React-Static |
OMG I'm so excited to see that the GitLab support has been released. Great work team on getting this out. Can't wait to see what's remaining now the Bitbucket side, because I would totally select NetlifyCMS over Forestry.io I really like the editorial flow in Netlify CMS compared to Forestry.io, so I'm keen as to test it out in beta and help resolve the PR. |
Any progress update on this? We've got a team of people that'd love to use this CMS, but bitbucket integration is a prerequisite for our use case. |
Closing in favor of #1504. |
Fixes #234
- Summary
Adds bitbucket api backend support at a basic level.
TODO to wrap up this WIP:
- Test plan
Change your cms admin config.yml file to point to a bitbucket repo. Setup oauth with bitbucket and an oauth server (or possibly netlify's oauth server). see example bitbucket client site project for example cms config
- Description for the changelog
Add basic bitbucket api backend support
- A picture of a cute animal (not mandatory but encouraged)
Photo by Waranya Mooldee on Unsplash