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

AzureDevops: fix automerge #1186

Closed

Conversation

oliverisaac
Copy link
Contributor

@oliverisaac oliverisaac commented Sep 12, 2020

As covered in #1152 , the user GUID must be set for the auto merge in Azure Devops to actually run.

@lkysow , this is a "hacky" solution to that problem. This is a proof-of-concept (though fully functional) PR. If you're okay with the direction this is headed I can add the expected/required config options/documentation.

I got carried away and this is a full PR now :)

owner, project, repoName := SplitAzureDevopsRepoFullName(pull.BaseRepo.FullName)
mergeResult, _, err := g.Client.PullRequests.Merge(
g.ctx,
owner,
project,
repoName,
pull.Num,
mergePull,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, and mergePull isn't actually used by the g.Client.PullRequests.Merge function call so we can set that to nil. If you want this could be in a separate PR, I feel that dropping the unnecessary setup of mergePull helps clarify the code.

@codecov
Copy link

codecov bot commented Sep 12, 2020

Codecov Report

Merging #1186 into master will decrease coverage by 0.17%.
The diff coverage is 34.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1186      +/-   ##
==========================================
- Coverage   70.71%   70.53%   -0.18%     
==========================================
  Files          71       71              
  Lines        5975     5983       +8     
==========================================
- Hits         4225     4220       -5     
- Misses       1398     1408      +10     
- Partials      352      355       +3     
Impacted Files Coverage Δ
server/server.go 60.70% <0.00%> (ø)
server/user_config.go 100.00% <ø> (ø)
server/events/vcs/azuredevops_client.go 60.46% <30.00%> (-5.09%) ⬇️
cmd/server.go 79.75% <100.00%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update affc936...a57706e. Read the comment docs.

@zparnold
Copy link

@oliverisaac I am the colleague of the person who opened this. We fixed it in our internal fork which I offered amonst other fixes in #1189 . Could you let me know if I went down the right path? It appears to be working for us internally. Microsoft Graph API descriptor objects are non-obvious and hard 😂

@oliverisaac
Copy link
Contributor Author

@zparnold , I love some of the changes y'all made, Especially adding the option to delete a source branch on merge!

I tried to close the branch using the GUID of the PR owner, but the PAT we gave to the atlantis app doesn't have permission to impersonate users and so wasn't able to close using anything but its own GUID. We set up a user account which acts as a "service account" in azdo and then generated a PAT in that user's profile so that we could lock down the account as much as possible. Any idea which permissions you gave your PAT so it could close as someone else?

@zparnold
Copy link

@Dilregore might be the person from our side who can answer that. I just fix the bugs he observes 😂

@Dilergore
Copy link

#1289

We created another PR which is using a brand new feature we added to the SDK to query the user ID dynamically.

@Dilergore
Copy link

#1289 solved this. You should probably abandon @oliverisaac

@lkysow
Copy link
Member

lkysow commented Jan 18, 2021

I'll close this then but if I'm wrong to have closed it, let us know and we'll reopen.

@lkysow lkysow closed this Jan 18, 2021
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.

4 participants