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

Support Pull Requests #739

Merged
merged 2 commits into from
Apr 17, 2021
Merged

Support Pull Requests #739

merged 2 commits into from
Apr 17, 2021

Conversation

marcinwyszynski
Copy link
Contributor

Use case

Frankly, I only came here to get a list of open pull requests from Terraform (the github_repository_pull_requests data source), but I wouldn't be able to test it properly without the ability to create PRs from Terraform, so here we go. My immediate use case for this feature is test/QA/preview (every company has their own name) environments automatically created from open Pull Requests and kept alive while these remain open. It goes something like this:

data "github_repository_pull_requests" "this" {
  base_repository = "infra"
  base_ref        = "main"
}

module "pr_stacks" {
  for_each = toset([ for pr in data.github_repository_pull_requests.this.results : pr.head_ref ])

  source = "./source"
  branch = each.key
}

The source module can then contain the definition of a Terraform project in form of a spacelift_stack or a tfe_workspace, pointing to the head branch of the PR. I'm sure there are plenty other use cases, but this is what I personally need it for.

Notes

Below are some of the notes I've been taking when researching and working on this diff. Hopefully they can shed some light on my decisions and help with the review.

Forks are supported

I assumed that the resource should support pull requests between forks. Not sure how frequent that use case would be in practice, but the extra cost was negligible. In the API, the "owner" field can be optionally supplied to explicitly indicating the owner of the repo to which we're trying to upstream our changes. I also called a field called "repository" in most other resources "base_repository" to make a clear distinction between (potentially) 2 repositories involved in creating a PR.

Deleting Pull Requests

It's not entirely clear how to treat PR deletion according to Terraform's CRUD semantics. The approach I've taken here is to close the PR unless it's already closed or merged in GitHub. Merging it (a "positive" action) feels intuitively wrong in what effectively is a destructor (a "negative" action).

Drafts

Creating drafts is not supported. While the v3 API call itself allows you mark the PR as draft when creating it, it does not allow you to change this setting afterwards. Hence, Terraform cannot manage the lifecycle of this field, and I decided to make it computed instead.

@jcudit jcudit added this to the v4.8.0 milestone Apr 6, 2021
@jcudit
Copy link
Contributor

jcudit commented Apr 6, 2021

🎉 excited for this one, thanks for the contribution!

I'm overall 👍🏾 on the data sources, but would like to think more about the new pull request resource. This project has considered adding a resource to manage Pull Requests in the past but shied away from it in favour of the github_repository_file resource. I suspect adding this resource will give users an attractive option and we'll end up getting more bug reports and feature requests to handle creative edge cases. Feels like something that comes with a heavier maintenance burden than other types of resources. Not against it, just hoping to hear more from the community around requirements so we can minimize re-work when initially rolling this out.

I've added this to the milestone after next and hope to hear more opinions around the implementation when revisiting. 🙇🏾 for the patience while this one works its way through the release process.

@marcinwyszynski
Copy link
Contributor Author

Thanks @jcudit, please LMK if I can be of any assistance.

Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

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

Thanks again for making these available for everyone! Excited to see how these resources get used in the wild.

@jcudit jcudit merged commit 1cb5703 into integrations:master Apr 17, 2021
@marcinwyszynski marcinwyszynski deleted the pull-requests branch April 17, 2021 16:18
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
* Support Pull Requests

* fixup! typo in description

Co-authored-by: Jeremy Udit <[email protected]>
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.

2 participants