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_pipeline_authorization - Allow pipeline authorization across projects #973

Conversation

josh-barker
Copy link
Contributor

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?

Currently, we cannot authorize pipelines cross project repository.

This PR adds a new optional property to specify the pipeline's project id.

Issue Number:

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

  • Yes
  • No

Does this introduce a breaking change?

  • Yes
  • No

Any relevant logs, error output, etc?

(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)

Other information

@xuzhang3
Copy link
Collaborator

@josh-barker In most case the project_id is same to pipeline_project_id. The only difference is when the resource type is repository the project_id will be used to generate the resource ID. We can remove the pipeline_project_id and delegate the generated resource ID to user(resource Id is <projectId>.<resourceId> when the resource type is repository), this would be a breaking change but acceptable

@josh-barker
Copy link
Contributor Author

@josh-barker In most case the project_id is same to pipeline_project_id. The only difference is when the resource type is repository the project_id will be used to generate the resource ID. We can remove the pipeline_project_id and delegate the generated resource ID to user(resource Id is <projectId>.<resourceId> when the resource type is repository), this would be a breaking change but acceptable

Hi @xuzhang3, I think it would be better as a consumer of the provider to add the additional optional property so I don't need to craft that string. project_id is already a required property and I can imagine that people could easily make mistakes putting in the wrong format or project id.

I could add another example to the documentation so people can understand the use case for that property, and expand on the description if you think that would help?

@xuzhang3
Copy link
Collaborator

@josh-barker I didn't consider cross-projects when I added this resource, the pipeline_project_id remind me that I should not generate the resource ID internally. It would be better to delegate control to the consumer.

Potential config:

resource "azuredevops_pipeline_authorization" "test" {
  project_id  = azuredevops_project.test.id
  resource_id = "${azuredevops_project.remote_repo.id}.${data.azuredevops_git_repository.remote_repo.id}"
  type        = "repository"
  pipeline_id = azuredevops_build_definition.test.id
}

@xuzhang3
Copy link
Collaborator

xuzhang3 commented Feb 26, 2024

@@josh-barker what I thinking is remove the hardcoded resource ID generated in the code and let the customer manage it themselves. This should cover your requirement.

@josh-barker
Copy link
Contributor Author

Hey @xuzhang3

As you said before, the majority of situations authorize a pipeline to a resource that belongs in the same AZDO project.
I think it's better to design the resource so the inputs fit the majority situation, and allow optional parameters for the edge cases.

As a user of the provider, I'd find it annoying to have to update all the pipeline authorizations in that format, where I could easily make a mistake - project id, missing full stop, etc.

If you're still not keen on the idea of an optional property, how about a specialised resource for cross pipeline/repo access? I think the repository is the only resource that can be given cross project access.

Thanks!

@xuzhang3
Copy link
Collaborator

@josh-barker We just need to remove this Repo resource ID code snippet to support cross project authorization. The repository ID composed in format projectID.repositoryId, expose this to consumer and let the consumer deicide which project and repository they want to authorize. The project ID in the resource ID can be the same or different from the project ID to which the pipeline belongs.

@plejon
Copy link

plejon commented Mar 12, 2024

Any ETA on this?

@xuzhang3
Copy link
Collaborator

Any ETA on this?

I create another PR #989 for this feature, a bit different from this PR. In #989 cx allows to compose the resource ID(type=respository) themselves but this will be a breaking change.

@plejon
Copy link

plejon commented Mar 14, 2024

This this merge be planned into a new major version or similar?

@xuzhang3
Copy link
Collaborator

This this merge be planned into a new major version or similar?

We plan to merge #989 instead of this PR

@josh-barker
Copy link
Contributor Author

Hi @xuzhang3 ,

It would be good to get other people's feedback about the proposed change in #989

Personally, I feel strongly that the provider should handle the construction the payloads for the APIs and simplifies the usage for us. I also prefer to minimise breaking changes so that upgrades are simple.

So I would expect that I only need to specify the project ID once when the repo and pipeline exist in the same project.

resource "azuredevops_pipeline_authorization" "same_project" {
  project_id  = azuredevops_project.test.id
  pipeline_id = azuredevops_build_definition.test.id

  type        = "repository"
  resource_id = data.azuredevops_git_repository.local_repo.id
}

However, when there is a cross project repo authorization, I'd expect I need to pass in the other project id.
This is what lead me to the following solution:

resource "azuredevops_pipeline_authorization" "another_project" {
  pipeline_project_id = data.azuredevops_project.remote_repo.id
  pipeline_id = azuredevops_build_definition.test.id

  project_id  = azuredevops_project.test.id
  resource_id = data.azuredevops_git_repository.remote_repo.id
  type        = "repository"  
}

I'd be totally happy to rename pipeline_project_id to something else. In fact, it might make more sense to rename it to resource_project_id or repository_project_id, as the subject of the resource is a pipeline.

For example:

resource "azuredevops_pipeline_authorization" "another_project" {
  project_id  = azuredevops_project.test.id
  pipeline_id = azuredevops_build_definition.test.id

  type        = "repository"
  resource_id = data.azuredevops_git_repository.remote_repo.id
  resource_project_id = data.azuredevops_project.remote_repo.id
}

Hope that makes sense and thanks for listening to my ideas

@plejon
Copy link

plejon commented Mar 15, 2024

What about adding a block instead of an extra argument.
Block target_resource to specify targeted resource.
And arguments resource_id and type cannot be used together with target_resource

resource "azuredevops_pipeline_authorization" "example" {
  project_id  = azuredevops_project.project1.id
#  resource_id = data.azuredevops_git_repository.project2_repo.id
#  type        = "repository"
  pipeline_id = azuredevops_build_definition.project1_pipeline.id
  target_resource {
    project_id  = data.azuredevops_project.project2.id
    resource_id = data.azuredevops_git_repository.project2_repo.id
    type        = "repository"
  }
}

@xuzhang3
Copy link
Collaborator

@josh-barker @plejon thanks for your suggestions. @josh-barker your implementation is the better choice. Not bring the breaking change. We can keep pipeline_project_id naming as this property reflect the project where the pipeline exist, although it is a bit duplicate of project_id

@xuzhang3 xuzhang3 changed the title fix: allow pipeline authorization across projects azuredevops_pipeline_authorization - Allow pipeline authorization across projects Mar 19, 2024
@xuzhang3 xuzhang3 merged commit 17a6c86 into microsoft:main Mar 19, 2024
3 checks passed
@josh-barker josh-barker deleted the fix/pipeline-authorizaton-on-external-project-repo branch March 21, 2024 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants