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

Azure Devops support #719

Merged
merged 73 commits into from
Oct 26, 2019
Merged

Azure Devops support #719

merged 73 commits into from
Oct 26, 2019

Conversation

mcdafydd
Copy link
Contributor

This pull request adds support for using Azure Devops as a VCS provider.

David McPike and others added 30 commits April 1, 2019 20:49
- remove ADHostname
- add ADOrg and ADProject
cmd/server.go:
- Add default empty strings for Azure Devops webhook basic user/pass

server/user_config.go:
- Rename AzureDevopsBasicUser to AzureDevopsWebhookBasicUser to signify
difference from the API call credentials

server/server.go:
- fix azuredevopsClient var
- add Azure Devops to []supportedVCSHosts
- add AzureDevopsRequestValidator for basic auth webhook support

server/azuredevops_request_validator*.go:
- add charset utf-8 to support content-types for webhooks
- start adding validator tests

server/events_controller.go*:
- change azuredevops webhook header to Request-Id
- move Validate() and Basic Auth webhook support to go-azuredevops
- add pull and comment event functions
- remove unneeded NewAzureDevopsClient() code
- change to using const refs
- add GetPullRequest()
- Pull request events
- Work item comment events
- Add missing headRepo set to AzureDevops event parser
- Strip 'refs/heads' from branch values sent to models.PullRequest
- Fix calls to NewClient() to use org, not username
- Add notes about use of /'s in repo FullName
- Force use of 'dev.azure.com' for VCSHost.Hostname value
- NewAzureDevopsClient(): Rename 'account' to 'username' to match docs
- NewAzureDevopsClient(): Rename unused 'hostname' to 'org'
- Add bluemonday to strip HTML tags from comments
- Rename all WorkItemEvent refs to WorkItemCommentedEvent
- Rename pullEvent to event to match types
- Minor comment clean-ups
- Support multiple pull request links in a work item
- Fixes logic error in PullIsMergeable()
- Replace GetRemoteURL() with GetWebURL() - returns what we want
- Remove project from NewRepo() calls
- Add new function SplitAzureDevopsRepoFullName
mcdafydd and others added 2 commits October 2, 2019 13:01
Co-Authored-By: Luke Kysow <1034429+lkysow@users.noreply.github.com>
@lkysow
Copy link
Member

lkysow commented Oct 2, 2019

Can you help explain why users would rather link and comment on a work item for each pull request rather than comment directly on the PR?

@mcdafydd
Copy link
Contributor Author

mcdafydd commented Oct 3, 2019

Can you help explain why users would rather link and comment on a work item for each pull request rather than comment directly on the PR?

I was following the idea in the Microsoft documentation here that recommends driving development from the work item, which seemed fine at the time given the lack of the PR comment webhook.

In our environment today, putting comments directly on the PR and more closely linking the Terraform plan with its execution makes sense. I can't think of a great reason we would need to run atlantis commands from a work item rather than a PR, but I imagine we will remain interested in linking our pull requests to a work item for tracking.

I'm happy to defer to more experienced teams doing this and adopt the recommended guidance. I started adding support for the new event payload to go-azuredevops. I don't suspect it'll take too much more work to finish it.

Thanks!

@mcdafydd
Copy link
Contributor Author

mcdafydd commented Oct 4, 2019

I deployed a new branch from my fork just now azure-devops-pr-comment. I'll test it out a bit. The pull request comments are threaded. I'm creating a new thread for each response from atlantis for now, rather than sending the atlantis response into the same thread with the command, since I don't have an easy way to pass the thread and parent comment ID from the webhook payload over to the vcs.CreateComment() function. Also, this code only supports Resource Version 2.0 of the webhook payload (which seems to be the default).

The branch would still need some code removed presuming we move forward with this method for responding to comments.

Edit: removing quoting

@lkysow
Copy link
Member

lkysow commented Oct 4, 2019

👍 I like going this route.

- Update docs
- Remove work item commented event code
@mcdafydd
Copy link
Contributor Author

mcdafydd commented Oct 5, 2019

@dedamico @naru014 @jpreese If you're interested in testing the changes, I'd love any feedback.
All work item comment connectivity is gone, so interaction is now solely through pull request creation, updates, and comments, similar to other VCS providers.

I ran into the issue yesterday where upon PR creation I immediately got back a comment saying the workspace was already locked. It was caused by a master branch policy that automatically adds reviewers. So when the PR was created, a "pullrequest.created" event was generated, and then about 1 second later a "pullrequest.updated" event was sent when the reviewers were added.

Is this a situation we want to improve in code at all or just document?

@lkysow
Copy link
Member

lkysow commented Oct 7, 2019

Is this a situation we want to improve in code at all or just document?

Can we find out that what changed was just new reviewers? We should only act on pull request update events that are about pushing a new commit (or force pushing). We should detect this before it gets to the stage where it ends up triggering locking.

@mcdafydd
Copy link
Contributor Author

mcdafydd commented Oct 8, 2019

Can we find out that what changed was just new reviewers? We should only act on pull request update events that are about pushing a new commit (or force pushing). We should detect this before it gets to the stage where it ends up triggering locking.

Ok, sounds good. Yes, I'm sure this is what triggered it. Looking at the webhook payloads, I think the primary way we can detect it is in the message text "changed the reviewer list". There's no clear field for this particular type of pullrequest.updated event type.

Here are the relevant log snippets. First, the PR opening webhook in the ADO event history:

Sent at: Saturday, October 5, 2019 12:41:20 AM
Message
Dev created pull request 11871 (PR title) in REPO

Atlantis receives it:

2019/10/05 00:41:20+0000 [INFO] middleware.go:38 server: POST /events – from 20.36.242.132:5952
2019/10/05 00:41:20+0000 [DBUG] events_controller.go:130 server: Handling AzureDevops post
2019/10/05 00:41:20+0000 [DBUG] events_controller.go:226 server: Request valid
2019/10/05 00:41:20+0000 [DBUG] events_controller.go:242 server: Handling as pull request event
2019/10/05 00:41:20+0000 [INFO] events_controller.go:547 server: Identified event as type "opened"
2019/10/05 00:41:20+0000 [INFO] events_controller.go:347 server: Executing autoplan
2019/10/05 00:41:20+0000 [INFO] middleware.go:42 server: POST /events – respond HTTP 200

Then the second PR updated event history log:

Sent at: Saturday, October 5, 2019 12:41:21 AM
Message
Dev changed the reviewer list for pull request 11871 (PR title) in REPO

and receipt:

2019/10/05 00:41:21+0000 [INFO] middleware.go:38 server: POST /events – from 20.36.242.132:5953
2019/10/05 00:41:21+0000 [DBUG] events_controller.go:130 server: Handling AzureDevops post
2019/10/05 00:41:21+0000 [DBUG] events_controller.go:226 server: Request valid
2019/10/05 00:41:21+0000 [DBUG] events_controller.go:242 server: Handling as pull request event
2019/10/05 00:41:21+0000 [INFO] events_controller.go:547 server: Identified event as type "updated"
2019/10/05 00:41:21+0000 [INFO] events_controller.go:347 server: Executing autoplan
2019/10/05 00:41:21+0000 [INFO] middleware.go:42 server: POST /events – respond HTTP 200
2019/10/05 00:41:21+0000 [DBUG] project_command_builder.go:102 org/project/REPO#11871: Got workspace lock
2019/10/05 00:41:21+0000 [WARN] project_command_builder.go:99 org/project/REPO#11871: Workspace was locked
2019/10/05 00:41:21+0000 [DBUG] project_command_builder.go:110 org/project/REPO#11871: 5 files were modified in this pull request
2019/10/05 00:41:21+0000 [INFO] working_dir.go:123 org/project/REPO#11871: Creating dir "/home/user/.atlantis/repos/org/project/REPO/11871/default"
2019/10/05 00:41:22+0000 [EROR] command_runner.go:410 org/project/REPO#11871: The default workspace is currently locked by another command that is running for this pull request–wait until the previous command is complete and try again

@lkysow
Copy link
Member

lkysow commented Oct 8, 2019

Looks like you're right. We can do a string.Contains to look for a message that the source branch was updated. Or we could require users specify the webhook change type to only be source branch updated. However I like not requiring users to set this and us just detecting the right types for them.

image

@mcdafydd
Copy link
Contributor Author

mcdafydd commented Oct 8, 2019

I just saw this feature too :). We would also need "Votes score changed" events for vcs.PullIsApproved(). I'm down to make it easier for users so will add something to match the event type and submit it soon.

@mcdafydd
Copy link
Contributor Author

mcdafydd commented Oct 9, 2019

There were at least six valid pull request updated message strings that we would've wanted to accept, so I just ignored the one message we don't want for now.

@yradsmikham
Copy link

Can we get an update on this? Would love to see it merge soon! Thanks!

- Create a Personal access token by following [https://docs.microsoft.com/en-us/azure/devops/organizations/accounts/use-personal-access-tokens-to-authenticate?view=azure-devops](https://docs.microsoft.com/en-us/azure/devops/organizations/accounts/use-personal-access-tokens-to-authenticate?view=azure-devops)
- Label the password "atlantis"
- The minimum scopes required for this token are:
- Code (Read)
Copy link
Member

Choose a reason for hiding this comment

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

Needs Code (Read & Write), I think for writing pr comments

@@ -71,7 +74,20 @@ func NewRepo(vcsHostType VCSHostType, repoFullName string, cloneURL string, vcsU
return Repo{}, errors.New("cloneURL can't be empty")
}

if !strings.HasSuffix(cloneURL, ".git") {
if vcsHostType == AzureDevops {
Copy link
Member

Choose a reason for hiding this comment

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

This was failing for me because my repo had a - in it. The regex could be fixed but is this necessary? Can a malformed repo name get in here?

err = errors.New("lastMergeSourceCommit.commitID is null")
return pullModel, baseRepo, headRepo, err
}
url := pull.GetURL()
Copy link
Member

Choose a reason for hiding this comment

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

We need to iterate through the links and pull out the web url. THis URL is the json url. CAn be done later though

{
  "resource": {
    "comment": {
      "_links": {
        "pullRequests": {
          "href": "https://dev.azure.com/lkysow/_apis/git/pullRequests/1" <<<< need this
        }
      }
    },

project = ""
}
viewData.RepoOwner = owner
viewData.RepoProject = project
Copy link
Member

Choose a reason for hiding this comment

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

This isn't actually used in the UI right now.

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

I think I can safely say that this is the best PR ever submitted to the Atlantis repo. Thanks for all your tireless work and bearing through the long review.

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.

8 participants