Automatically include code reviewers in a git repo #29

EliiseS · 2020-07-02T14:01:47Z

Migrated from #364
Originally created by @ferrandinand on Fri, 05 Jun 2020 10:43:24 GMT

Community Note

Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

We would like to add the possibility to configure code reviewers for a repository created in Azure Devops as it is explained in the official documentation (references bellow)

New or Affected Resource(s)

azuredevops_code_reviewers

Potential Terraform Configuration

resource "azuredevops_code_reviewers" "p" {
    required-reviewer-ids = ...  // list | required | ForceNew := false
    blocking = ..  // bool | optional | ForceNew := false | OneOf: true, false (default true)

    # can be defined 0 or many times
    scope {
        repository_id = ...  // string | required | ForceNew := false
        repository_ref = ... // string | required | ForceNew := false
        match_type = ...     // string | optional | ForceNew := false | OneOf: "Exact" (default), "Prefix"
    }
    submitter_can_approve = ... // bool | optional | ForceNew := false | OneOf: true, false (default)
}

References

https://docs.microsoft.com/en-us/azure/devops/repos/git/branch-policies?view=azure-devops#automatically-include-code-reviewers

https://docs.microsoft.com/en-us/cli/azure/ext/azure-devops/repos/policy/required-reviewer?view=azure-cli-latest

EliiseS · 2020-07-02T14:01:49Z

Migrated from #364 (comment)
Originally created by @C123R on Sun, 28 Jun 2020 16:25:34 GMT

Hi @C123R Sorry for reply late. Thanks for your participate. We have migrated this repository to terraform-provioder. I will transfer the issues to the new repository. But you can start your work. New Repository Address

@xuzhang3 @nmiodice just one more question, I have another WIP pull request #306 which is currently blocked due to AZDO GO SDK. Should I create (copy of)an issue in the new repo?

EliiseS · 2020-07-02T14:01:49Z

Migrated from #364 (comment)
Originally created by @xuzhang3 on Sun, 28 Jun 2020 03:44:22 GMT

Hi @C123R Sorry for reply late. Thanks for you participate. We have migrated this repository to terraform-provioder. I will transfer the issues to the new repository. But you can start your work. New Repository Address

EliiseS · 2020-07-02T14:01:49Z

Migrated from #364 (comment)
Originally created by @xuzhang3 on Mon, 29 Jun 2020 02:16:23 GMT

Hi @C123R I will transfer the whole issues to new repo later. You can create a new issue in the new repo too, I will handle the duplicate issues.

EliiseS · 2020-07-02T14:01:49Z

Migrated from #364 (comment)
Originally created by @C123R on Sat, 27 Jun 2020 21:16:36 GMT

Hey @xuzhang3 👋, can I give a shot?

xuzhang3 · 2020-07-03T02:44:29Z

Hi @C123R Can you comment on this issue so I can assign you this issue? :D

C123R · 2020-07-03T06:28:55Z

ping

C123R · 2020-07-03T07:30:24Z

@nmiodice Since you have worked on other branch policy resources.
From my point of view, it doesn't make sense to add another resource azuredevops_branch_policy_auto_reviewers with just one additional setting auto_reviewer_ids over azuredevops_branch_policy_min_reviewers.

I would suggest renaming azuredevops_branch_policy_min_reviewers to azuredevops_branch_policy_code_reviewers which will include 3 policy settings and reviewer_count as a required field:

resource "azuredevops_branch_policy_code_reviewers" "codereviewer" {
  project_id = azuredevops_project.project.id
  settings {
    reviewer_count    = 1       #required 
    submitter_can_vote = true
    auto_reviewer_ids = [
       "XXXXXXXX-5243-65e6-8139-XXXXXXXXXX",
        "XXXXXXXX-5243-65e6-8139-XXXXXXXXXX",
    ]
    scope {
      repository_id  = azuredevops_git_repository.repository.id
      repository_ref = azuredevops_git_repository.repository.default_branch
      match_type     = "Exact"
    }
  }
}

Considering this as a breaking change, sure we can deprecate existing resources and add another one as suggested.

What do you think?

CC @xuzhang3 @EliiseS

EliiseS · 2020-07-06T13:31:13Z

@C123R I think that's good idea! I'd go for the name azuredevops_branch_policy_reviewers though, because code seems a bit redundant

C123R · 2020-07-06T13:39:41Z

@C123R I think that's good idea! I'd go for the name azuredevops_branch_policy_reviewers though because code seems a bit redundant

@EliiseS Sounds good to me, I will come up with the PR. I guess then we need to mark azuredevops_branch_policy_min_reviewers as deprecated?

EliiseS · 2020-07-06T14:04:14Z

@C123R Yeah, let's do that for now. We can consider straight up removing it since we're still in pre-release versions of the provider, but I'd like others to weigh in on that @nmiodice @xuzhang3.

C123R · 2020-07-08T18:47:54Z

@EliiseS Sorry, I was wrong. I think we need to stick with the different resources for min_reviewers and auto_reviewers.

Reason: Even though both the policies are using the same endpoint and policy configuration, they have different policy type reference. For instance, the below request would be rejected with error The update is rejected by the policy.

{
  "isEnabled": true,
  "isBlocking": false,
  "type": {
    "id": "fa4e907d-c16b-4a4c-9dfa-4906e5d171dd"  # typeRef for minimum reviewers
  },
  "settings": {
    "minimumApproverCount": 1,
    "creatorVoteCounts": false,
    "requiredReviewerIds": [                                             # autoreviwers id
      "1d1dad71-f27c-4370-810d-838ec41efd41"
    ],
    "scope": [
      {
        "repositoryId": null,
        "refName": "refs/heads/master",
        "matchKind": "exact"
      }
    ]
  }
}

I will continue with the azuredevops_branch_policy_auto_reviewers. Sorry for the confusion.

EliiseS · 2020-07-08T18:54:33Z

@C123R I'm not 100% I understand, but I think you mean to say their request objects are different, right?

I think it would still be worth it to add them under one, similar to how I'm doing here: https://github.com/terraform-providers/terraform-provider-azuredevops/pull/17

C123R · 2020-07-09T14:50:33Z

@C123R I'm not 100% I understand, but I think you mean to say their request objects are different, right?

Ahh sorry. So, yeah your guess was right. The request objects are different, every branch policy has a unique policy type ref and respective settings. @nmiodice have nicely explained it here.

I think it would still be worth it to add them under one, similar to how I'm doing here: #17

I had a quick look at your PR, I think you are conditionally changing the request body, right?. But that's not something we need it here. Let me try to explain with an example:

Considering azuredevops_branch_policy_reviewers is the new resource which supports multiple policy configuration:

resource "azuredevops_branch_policy_reviewers" "codereviewer" {
  project_id = azuredevops_project.project.id
  settings {
    reviewer_count    = 2                                              #  min_reviewer count
    submitter_can_vote = true                                                       
    auto_reviewer_ids = [                                              # required_reviewers
       "XXXXXXXX-5243-65e6-8139-XXXXXXXXXX",
        "XXXXXXXX-5243-65e6-8139-XXXXXXXXXX",
    ]
    scope {
      repository_id  = azuredevops_git_repository.repository.id
      repository_ref = azuredevops_git_repository.repository.default_branch
      match_type     = "Exact"
    }
  }
}

Here, in this case, we need to make 2 POSTcalls to https://dev.azure.com/{org}/{project}/_apis/policy/configurations with body:

MinReviewerCount:

 {
  "type": {
    "id": "fa4e907d-c16b-4a4c-9dfa-4906e5d171dd"  # typeRef for minimum reviewers
  },
  "settings": {
    "minimumApproverCount": 2,
    "scope": [..]
  }
}

Required reviewers:

  {
  "type": {
    "id": "fd2167ab-b0be-447a-8ec8-39368250530e"  # typeRef for requiredReviewer
  },
  "settings": {
    "requiredReviewerIds": [                        
      "1d1dad71-f27c-4370-810d-838ec41efd41"
    ],
     "scope": [..]
  }
}

Sure. The creation of multiple policies is not that complicated, but my confusion or why I see its complicated(maybe due to lack of experience in terraform providers):

How are we gonna deal with the ID of such a resource? as per my understanding, (also in current implementation azuredevops_branch_policy_min_reviewers or azuredevops_branch_policy_build_validation) we are setting policy configuration ID from the response.

EliiseS · 2020-07-09T15:41:34Z

@C123R Ah, I see now! You're correct, in that case I think it's best to split them like you intend to :)

Looking forward to the PR!

xuzhang3 · 2020-07-21T09:18:37Z

Close this issue for #71

EliiseS added the new feature label Jul 2, 2020

EliiseS removed the new feature label Jul 2, 2020

xuzhang3 added the new-resource label Jul 3, 2020

xuzhang3 assigned C123R Jul 3, 2020

C123R mentioned this issue Jul 12, 2020

feat: Auto reviewer branch policy #71

Merged

11 tasks

xuzhang3 closed this as completed Jul 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically include code reviewers in a git repo #29

Automatically include code reviewers in a git repo #29

EliiseS commented Jul 2, 2020

EliiseS commented Jul 2, 2020

EliiseS commented Jul 2, 2020

EliiseS commented Jul 2, 2020

EliiseS commented Jul 2, 2020

xuzhang3 commented Jul 3, 2020

C123R commented Jul 3, 2020

C123R commented Jul 3, 2020 •

edited

Loading

EliiseS commented Jul 6, 2020

C123R commented Jul 6, 2020

EliiseS commented Jul 6, 2020

C123R commented Jul 8, 2020

EliiseS commented Jul 8, 2020

C123R commented Jul 9, 2020

EliiseS commented Jul 9, 2020

xuzhang3 commented Jul 21, 2020

Automatically include code reviewers in a git repo #29

Automatically include code reviewers in a git repo #29

Comments

EliiseS commented Jul 2, 2020

Community Note

Description

New or Affected Resource(s)

Potential Terraform Configuration

References

EliiseS commented Jul 2, 2020

EliiseS commented Jul 2, 2020

EliiseS commented Jul 2, 2020

EliiseS commented Jul 2, 2020

xuzhang3 commented Jul 3, 2020

C123R commented Jul 3, 2020

C123R commented Jul 3, 2020 • edited Loading

EliiseS commented Jul 6, 2020

C123R commented Jul 6, 2020

EliiseS commented Jul 6, 2020

C123R commented Jul 8, 2020

EliiseS commented Jul 8, 2020

C123R commented Jul 9, 2020

EliiseS commented Jul 9, 2020

xuzhang3 commented Jul 21, 2020

C123R commented Jul 3, 2020 •

edited

Loading