feat(backend-azure): ms devops support #3318
Conversation
|
Thanks @nathankitchen! Very cool contribution. As for previews you can see an example here: The feature works by getting the Pull Request head commit status and searching for a
You can probably test by creating a status manually using the api: I'll add some more comments on the PR soon. |
| ); | ||
| } | ||
|
|
||
| const components = trim(location, '/').split('/', 3); |
There was a problem hiding this comment.
Would be nice to validate that location is indeed in the format organisation/project/repo
| } | ||
| } | ||
|
|
||
| export interface AzureUser { |
There was a problem hiding this comment.
I like all these type definitions 😄
There was a problem hiding this comment.
Out of interest, would you consider it good practise to do class-per-file? I considered creating a few folders as modules and organising like that, but didn't want to break with what looked like wider convention. I'm usually a .NET dev, old habits die hard : )
There was a problem hiding this comment.
I'm up for separating classes to different files once its needed. Doesn't seems like the case here as they are only used in one place.
| return `${CMS_BRANCH_PREFIX}/${contentKey}`; | ||
| } | ||
|
|
||
| async getMergeRequests(sourceBranch?: string) { |
There was a problem hiding this comment.
We should use "pull requests" naming along this file if that's what the API uses.
| oldObjectId: string; | ||
| }; | ||
|
|
||
| class AzureRef { |
There was a problem hiding this comment.
Can we use plain JavaScript objects instead of classes?
Looks like these classes are just containers for properties.
| constructor(config: AzureApiConfig, token: string) { | ||
| this.repo = config.repo; | ||
| this.apiRoot = trim(config.apiRoot, '/') || 'https://dev.azure.com'; | ||
| this.endpointUrl = `${this.apiRoot}/${this.repo?.org}/${this.repo?.project}/_apis/git/repositories/${this.repo?.name}`; |
There was a problem hiding this comment.
Should we change repo to be a mandatory attribute in the config?
| } | ||
|
|
||
| async updatePullRequestLabels(mergeRequest: any, labels: string[]) { | ||
| mergeRequest.labels.forEach(async (l: AzurePRLabel) => { |
There was a problem hiding this comment.
Using async inside forEach doesn't behave the way you expect it to be.
Also the next forEach seems redundant as we can just POST the CMS label.
A simpler way to implement this (since it seems the azure API doesn't support updating all labels in one request) is to change updatePullRequestLabels to replaceLabel and pass in toReplace and newLabel, delete the old one and post the new one (or something similar).
| return this.api!.listFiles(collection) | ||
| .then(files => { | ||
| if (extension) { | ||
| return files.filter(file => file.relativePath.endsWith('.' + extension)); |
There was a problem hiding this comment.
Consider using a utility function like here:
https://github.com/netlify/netlify-cms/blob/684b79e43bebb63ce1e844eae5c8c0e76087687b/packages/netlify-cms-backend-bitbucket/src/implementation.ts#L258
Also depth is important when nesting directories via:
https://www.netlifycms.org/docs/beta-features/#folder-collections-path
| } | ||
| return files; | ||
| }) | ||
| .then(this.fetchFiles); |
There was a problem hiding this comment.
See https://github.com/netlify/netlify-cms/blob/684b79e43bebb63ce1e844eae5c8c0e76087687b/packages/netlify-cms-backend-bitbucket/src/implementation.ts#L261 for using the utility method entriesByFolder to avoid duplication of fetchFiles (https://github.com/netlify/netlify-cms/blob/684b79e43bebb63ce1e844eae5c8c0e76087687b/packages/netlify-cms-lib-util/src/implementation.ts#L166)
|
@nathankitchen, I added some comments. I will do some testing later on. |
|
I did a bit of testing on this branch this morning and things look really good. The one thing that I'm having a problem with is that if I create a new collection I get a big nasty error about not being able to find the directory in git. However if I simply ignore the error I can create a new entry in that collection and upon publish I get a similar big nasty error. If I refresh the page (note it doesn't work to navigate away and back has to be a refresh) then I see the entry I created and can edit/add more entries without a problem. I've got a bit of time this morning so I'll take a look and see if I can provide some code to fix it. Error upon first navigation to the collection is Error upon save is |
|
correction to previous post: I have a simple codefix for the first error. Need to more thoroughly regression test it, but I think its good to go. The second error happens on EVERY save of a new entry. Updates are fine, but all new entries throw that. Working through a solution now. |
Thanks, we needed to added code in the other backends not to fail on empty directories (which usually happens with new collections). |
|
Still trying to figure out why, but saving is a problem with the editorial workflow. As long as I don't use the editorial workflow everything is fine. The underlying problem is that the api returns a 404 after the merge completes. It won't return a the file until some point later. Logging out/in then returns the file but there has to be some better trigger to get the file to return. I'm thinking it has to do with local vs remote state of the branches from what I'm seeing in the logs but possibly that is a wild goose chase. So my question for @erezrokah, should we be expecting editorial workflow to work? From reading the docs it may be something that is github specific right now? |
@jburich If the code is there it should work. Editorial workflow works for other backends (in beta) and not just GitHub. Docs were recently updated to reflect that: We could remove the code to support editorial workflow and report an error if someone uses it with azure (this was the case for GitLab and BitBucket before support was added), but I don't think we're that far ahead from making it work. Let's wait for the response of @nathankitchen. |
|
Ok cool. I’m willing to take some time to get it working, just didn’t want to invest time in something that could potentially be not-ready yet at a higher level. I’ll look into this, but @nathankitchen, let me know if you already have thoughts around it.
|
GitLab supports both Web Application Flow and implicit authentication. I Don't think it is a must for this PR as long as we document it. |
|
@erezrokah I'm looking at the Preview Links now. I just want to confirm that I'm following correctly ... Those status are the statuses from the Pipelines, ie the CI build, correct? And we would expect that context and target_url to tell us where the pipeline is pushing the code, correct? |
For question 1, correct, those are statuses set from the Pipelines, ie the CI build. For question 2, In the case of the CMS a relevant context is one that has If that still doesn't make any sense leave and I'll try to figure it out. Not sure Azure pipelines has a concept of previews (I'm guessing one needs to set it up manually, hence the confusion). |
|
That's perfect. That actually makes sense now. I'll set up a pipeline today and see what azure gives us.
|
* Ensuring that the code works when there is nothing in the repository * fix: handle the case were git is empty * fix: wait for azure to complete a pull request Azure pull requests are async and go into a queued state when you complete them. To wait for it to actually complete you must get the pull request again and check its stauts. Once it status changes from 'queued' to 'complete' you can move on * fix(lint): formatting and typing * fix: unit tests Co-authored-by: edvedafi <[email protected]>
|
@jburich is there anything we can do to push this forward? I don't mind finalising it myself |
|
sorry I was stuggling with my client to get the pipelines set up then when they finally got out there the country went on lock down and its been chaos around here :) I have done almost no work on getting the deploy previews to work and the client has decided that the portion of the site we are going to use Netlify for is ok to move to Post-Launch so I'm hoping to get back to this stuff yet this this week but realistically its probably next week at the earliest when I have work time I can dedicated it. And I was contributing in my off hours, but now that my kids are home all day I spend my spare hours home schooling instead. |
|
@jburich, sorry didn't mean to rush anything, just wanted to make sure the all the work put into won't go to waste. Just let me know if there is anything the CMS team can do to make it easier. Of course the most important thing is for all of us to be safe during these times. |
|
No worries, just wanted to make sure your knew it wasn't abandoned :)
I'll dive back into the deploy previews in the next few days and let you know.
|
|
hey @erezrokah / @nathankitchen I just wanted to reach out and let you know I haven't forgotten about this :) I still need it for the project I'm working on but other features ended up with a higher priority and covid19 is causing us to move a little slow. I do hope to get back to it soon, but it may be another week or two. In the meantime I think the only thing left to do is deploy previews and other than some research and learning I haven't done any actual work. In all honestly I'll probably have to re-learn it after a month way ;) So if either of you are itching to close out this PR and wants to take that on go for it, otherwise I'm happy to do it as soon as I can, but like I said it may be another week or so waiting on me to get started. |
erezrokah
added
the
type: feature
|
is there anything I can help with here ? I used @nathankitchen s branch to play/test his version - and I got it integrated into dev.azure.com - which starts a build pipeline on content commit (release), builds and deploys a site to blob storage, which deploys into a CDN - which got flushed at the end of the pipeline when blob is completely updated - would be great if we could bring all pieces together and allow everyone to use this environment |
|
@chrismade, I think there is some work to be made to align this code with the other backends. |
|
@erezrokah pls allow me one more question - currently it seems that 3 files are out of sync which seems to be a feasible task to fix (without any detailed analysis) - but I cannot read from this PR what was the reason why @nathankitchen PR (27.Feb) and @jburich contribution in April never made this PR go into the final release and so avoiding future out-of-syncs. @jburich hat worked on something which he could not finish either because too challenging or simply due to other prios - if now anyone invests time to rebase how likely (what is blocking?) is it this time this whole effort ends in the official release? |
|
Each backend implements the following interface https://github.com/netlify/netlify-cms/blob/c33db099ac56edfd993de0212a8085d8d9243ce4/packages/netlify-cms-lib-util/src/implementation.ts#L98 that had changed (not in a major way, but it did change) so other than rebasing and resolving conflicts it would require adjusting the new azure backend implementation to the new interface. Also, I don't think the work on preview URLs was finalised. If someone would like to pick this up and finalise it we would be happy to have it in the official release. |
|
Hey guys, I have rebased the feature branch on my personal fork. I could work on the features that are still missing, but I need some pointers I have started adjusting the implementation class to the new |
|
@MKrupauskas that's amazing! Also, you could create a PR with your changes and post a guide on how to set it up with azure so the patient watchers of this PR can test it out 😄 Once you feel it is ready just assign/tag me and I'll go over it. |
|
Hello, @erezrokah I work with @jburich and have some time to dedicate to this. It looks like we need to rebase @MKrupauskas's copy of this branch on top of the latest master branch, resolve conflicts, make sure the e2e tests are passing (currently 1 is failing for |
|
I think we have a working solution for this which builds on all the great work that has been done by everyone here. Is there a way for me to push work from my personal fork of this branch directly to this PR? or should I make a new PR? |
You can make a new PR and we can close this one. Thanks @benhulan! |
|
Got it. I think I may have figured out why the editorial_workflow hasn't been working. I will spend a little more time on this and hope to be able to submit a separate PR for this very soon! |
|
Closing in favour of #4427 |
Fixes #1181
Summary
Azure DevOps (formerly Visual Studio Online) is a popular platform for hosting Git repositories. This PR includes an additional backend to enable Netlify-CMS to work with Azure DevOps repositories. It interacts with the Azure DevOps Git API using an implicit-auth token from Azure AD. Backend support includes:
I was encouraged to submit a PR in the Slack channel to get feedback from maintainers. Currently missing:
Test plan
Cypress tests to be produced (@erezrokah offered to complete these - thanks!)
Non-mandatory cute animal:
