Permission management

[tmeckel]

All Submissions:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.
• My code follows the code style of this project.
• I ran lint checks locally prior to submission.
• Have you checked to ensure there aren't other open PRs for the same update/change?

What about the current behavior has changed?

Initially filed as Pull Request at #238

Issue Number:

• Closes Fix to support project creation on Azure Devops "Server" #104
• Closes Build definition allow access in .tfstate inconsistent with TF configuration #153
• Closes Implement Terrafrom resource azuredevops_project_permissions #47
• Closes Implement Terrafrom resource azuredevops_git_permissions #46

The current implementation of the provider does not support the management of permission for project and git repositories.

This PR provides an implementation for two new resources (azuredevops_project_permissions, azuredevops_git_permissions) as described in the issues mentioned above and provides a framework for an easy implementation for other permission related Terraform resources (Security Namespace).

In addition the single-value data source azuredevops_git_repository has been added.

Does this introduce a change to go.mod, go.sum or vendor/?

• Yes
• No

• Upgraded terraform-plugin-sdk to v1.10.0, to have the ability to use RequiredWith validation in resources schema.
• Removed reference to github.com/microsoft/terraform-provider-azuredevops

Does this introduce a breaking change?

• Yes
• No

Any relevant logs, error output, etc?

[N/A]

Other information

[tmeckel]

@nmiodice, @xuzhang3 I changed the scripts/unittest.sh (924eedc) and scripts/acctest.sh (35d9884) scripts so that the build pipeline will not fail on skipped tests.

[tmeckel]

@EliiseS @nmiodice @xuzhang3 In addition I fixed some linting errors in azuredevops/internal/service/taskagent/resource_variable_group.go (9660e17)

[EliiseS]

It looks great! But the build is broken and I think we should have a build that runs the test before we can merge this. The build is being blocked by #10

[tmeckel]

It looks great! But the build is broken and I think we should have a build that runs the test before we can merge this. The build is being blocked by #10

@EliiseS I totally agree, but I've got the impression that we already lost momentum here again. Since the provider has been migrated to Hashicorp I don't see much progress. It got kinda silent here. Not much from @nmiodice or @xuzhang3 either. The build pipeline has to be fixed QUICKLY! Thought about removing the log messages from the Skip function be replacing them with SkipNow which hopefully let the build command pass in the pipeline, but I don't wanna change too much code, where the root cause lies somewhere else.

Who is working on #10? Nick? @xuzhang3? You? Someone from Hashicorp? A slightly unsatisfactory situation right now again. 😞

[xuzhang3]

@tmeckel Can we split this huge PR into small PRs? Dependency upgrade can be included in separate PR. One PR for one feature or function will be great.

[tmeckel]

@tmeckel Can we split this huge PR into small PRs? Dependency upgrade can be included in separate PR. One PR for one feature or function will be great.

I know the pull request is big, but to break it up into small pieces again would take a lot of time for me and I mostly don't get paid for this work, especially for work that's related to get a PR merged, because the customers I'm working with, use an own version of the provider that has those changes incorporated already.

[tmeckel]

HALLELUJAH!!! The pipeline passed 🚀 Well ... without acceptance tests ... Not that great that reviewers currently must execute the acceptance tests locally. 😞 Requesting review @EliiseS , @xuzhang3 @nmiodice

Oh and I again had to update the vendor directory. Added 50 files to the number of Files changed ... the number is insane already 😁 💥

[xuzhang3]

@tmeckel I will be late for this PR. I'm busy this week.

[tmeckel]

@xuzhang3 Thanks for letting me know

[xuzhang3]

Is this config was just for verify if terraform apply works? No check is configured for this step

[tmeckel]

I simply copied the code that @nmiodice created to test the multi value data source in azuredevops/internal/acceptancetests/data_git_repositories_test.go If there is an issue here we have to check about the acceptance test in this data source as well.

[xuzhang3]

Hi @tmeckel I need some extra test for this huge PR 🍡. I found bug when I execute the example configuration script in git_permissions, I'm need investigate this issue.

Steps to reproduce

• execute terraform apply --auto-approve
• change some configuration
• execute terraform apply --auto-approve again

[tmeckel]

Hi @tmeckel I need some extra test for this huge PR 🍡. I found bug when I execute the example configuration script in git_permissions, I'm need investigate this issue.

Steps to reproduce

* execute `terraform apply --auto-approve`

* change some configuration

* execute `terraform apply --auto-approve` again

I can reproduce the error on my side.

[tmeckel]

Hi @tmeckel I need some extra test for this huge PR 🍡. I found bug when I execute the example configuration script in git_permissions, I'm need investigate this issue.

Steps to reproduce

* execute `terraform apply --auto-approve`

* change some configuration

* execute `terraform apply --auto-approve` again

@xuzhang3 if you remove the default_branch property it all works as expected. Oh you don't have to change anything in the HCL code, the error occurs while refreshing the state. So I suspected resourceGitRepositoryRead to be the culprit, but I don't see any reference to the default_branch property. That's odd ..

Finally I could track down that resourceGitRepositoryUpdate is called, regardless if you changed anything for the Git repository resource. clients.GitReposClient.UpdateRepository returns the error message.

[tmeckel]

@xuzhang3 Mistery solved. The value for the property default_branchmust always be the complete Git path to the branch. So for branch master this is refs/heads/master and not simply master. This also explains why Terraform wants to update the Gi repository resource. The REST API returns a different value for the default_path property than the HCL definition. So from my point of view we have two options here

1. Update the documentation of the Git Resource that the value for the default_path property has to always the complete Git path of the branch and not only it's name, or
2. Make code more resilient about the fact that users only specify the name of the branch. Like if the value does not start with refs/heads prepend this to the value

Presumably you don't wanna add me option 2. to this monster 👹 PR 😁

Nevertheless I update the sample in the documentation around azuredevops_git_permissions

[xuzhang3]

@tmeckel Thanks for your PR, all feature is OK.
For git permission management, I'd like to mention that if user set Inheritance opened, some configure may override by inherit permission settings. This switch is opened by default.

Example:
use the example configure in git_permission. I'm the admin, I cannot change the CreateBranch to NotSet If Inheritance is open, the CreateBranch can only be set to allow ordeny. This may get users confused. Can you update the document that if Inheritance is open, permission configure may override by inherit.

[tmeckel]

@tmeckel Thanks for your PR, all feature is OK.
For git permission management, I'd like to mention that if user set Inheritance opened, some configure may override by inherit permission settings. This switch is opened by default.
Example:
use the example configure in git_permission. I'm the admin, I cannot change the CreateBranch to NotSet If Inheritance is open, the CreateBranch can only be set to allow ordeny. This may get users confused. Can you update the document that if Inheritance is open, permission configure may override by inherit.

@xuzhang3 I think you refer to this behavior, right? I mean that if you reset a defined value to NotSet on a lower level, the permission will be set to the permission the principal gets assigned from a higher level in the web interface.

The value NotSet is explicitly intended to set the value of a permission to the parent value if inheritance is enabled, but you definitely define an ACE on that particular level (eg on a Git branch), even if the ACE is empty for a particular action (no values in allow and deny). I think a piece of code from the namespaces.go file makes this clear:

To emphasize it a bit more: every time you define a azuredevops_git_permission resource you'll create an ACE in the ACL for the security namespace token.

The fact that the web interface of Azure DevOps is nice enough to always show the user (admin) the effective permissions for an identity at one level may require explanation but is technically irrelevant for the provider.

Oh and not to forget, if you want to reset a allowor deny permission to the inherited permissions of a principal you must set the value to NotSet for an security namespace action in HCL.

And I don't want to be mean, but I can't find a single word about this whole topic in the official documentation of the REST API. So why should we make up for this deficit on our side?

[tmeckel]

@xuzhang3 @EliiseS @nmiodice WOOOOHOOOOOO!!! Finally done!! 😃 👍 Thanks for never giving up, while reviewing this massive PR!