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

Provide "require signed commits" on github_branch_protection #214

Merged

Conversation

bltavares
Copy link
Contributor

@bltavares bltavares commented Apr 24, 2019

This commit provides changing the toggle to require signed commits on branch protection.

The API for signed commits is separate endpoint from the branch protection url, and does not come on the same payload. This means that we need to make an extra request to read and modify it.

Closes https://github.com/terraform-providers/terraform-provider-github/issues/87

@ghost ghost added the size/M label Apr 24, 2019
@bltavares bltavares changed the base branch from sdk-v0.12-beta2 to master May 2, 2019 22:18
@bltavares bltavares force-pushed the require-signed-commits-v12-beta branch from fb80993 to eaa247d Compare May 2, 2019 22:20
@bltavares bltavares changed the title [v0.12-beta2] Provide "require signed commits" on github_branch_protection Provide "require signed commits" on github_branch_protection May 2, 2019
This commit provides changing the toggle to require signed commits on
branch protection.

The [API for signed commits](https://developer.github.com/v3/repos/branches/#get-required-signatures-of-protected-branch)
is separate endpoint from the branch protection url, and does not come on the same payload.
This means that we need to make an extra request to read and modify it.

This changes requires `go-github` at least v19.

Need:
 - [ ] Update `go-github` version: https://github.com/terraform-providers/terraform-provider-github/pull/207
 - [ ] Provide default required reviewers count to avoid GitHub update error: https://github.com/terraform-providers/terraform-provider-github/pull/211
Closes https://github.com/terraform-providers/terraform-provider-github/issues/87
@bltavares
Copy link
Contributor Author

@paultyng I've updated this PR to use the latest changes recently landed on master. It would be really nice to have this landed so we can upgrade to the new release of the provider and drop the fork.

@paultyng
Copy link
Contributor

paultyng commented May 2, 2019

@bltavares would it be possible to add acceptance test coverage and docs for this?

@bltavares
Copy link
Contributor Author

I'm not used to acceptance tests on Terraform. Should I include it on the current branch protection test, or create a new one?

@paultyng
Copy link
Contributor

paultyng commented May 6, 2019

@bltavares adding to the existing one is probably fine if you don't think there is a reason to have an additional test. I'm not sure what the API interactions look like and if it would be useful to have tests turning it on and off, etc.

@paultyng paultyng self-assigned this May 6, 2019
@paultyng paultyng added the Type: Feature New feature or request label May 6, 2019
@ghost ghost added the Type: Documentation Improvements or additions to documentation label May 10, 2019
@bltavares
Copy link
Contributor Author

@paultyng I've changed the test to create the signed commit requirement, then remove it, as well as documentation.

Let me know if they are good enough.

@tobypinder
Copy link
Contributor

tobypinder commented Jun 18, 2019

Hey folks, any update on this? From my perspective, automating the branch protection rules is the most fiddly thing to standardise in the UI and we're very keen to add the signed_commit stuff to our internal state management as well 👍

We're not so au-fait with terraform provider dev but please let me know if any help is needed to get this over the line 🎉

@tracypholmes tracypholmes self-assigned this Jun 27, 2019
@tracypholmes
Copy link
Contributor

@bltavares @tobypinder we're reviewing it this week. Thank you for being patient with us!

@tracypholmes tracypholmes merged commit 832ea7e into integrations:master Jun 27, 2019
@maelvls
Copy link

maelvls commented Feb 18, 2020

Weird, as of today the official docs regarding the resource github_repository do not mention require_signed_commits although it has been released 2 weeks ago in v2.3.2.

@jcudit
Copy link
Contributor

jcudit commented Feb 24, 2020

@maelvls it looks like the documentation updated landed on a different resource. Mention me on any PRs that would fix the documentation flow for you in this use case.

kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
…rations#214)

* Provide "require signed commits" on `github_branch_protection`

This commit provides changing the toggle to require signed commits on
branch protection.

The [API for signed commits](https://developer.github.com/v3/repos/branches/#get-required-signatures-of-protected-branch)
is separate endpoint from the branch protection url, and does not come on the same payload.
This means that we need to make an extra request to read and modify it.

This changes requires `go-github` at least v19.

Need:
 - [ ] Update `go-github` version: https://github.com/terraform-providers/terraform-provider-github/pull/207
 - [ ] Provide default required reviewers count to avoid GitHub update error: https://github.com/terraform-providers/terraform-provider-github/pull/211
Closes https://github.com/terraform-providers/terraform-provider-github/issues/87

* Include tests

* Adds documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Type: Documentation Improvements or additions to documentation Type: Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] support requiring "signed commits" on github_branch_protection
6 participants