-
Notifications
You must be signed in to change notification settings - Fork 770
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
Revert Config Changes To github_branch_protection
#570
Conversation
`repository` was previously removed in favour of `repository_id`. We revert to `repository` here and transition `repository_id` to a computed value that is derived from a GraphQL lookup against the provided `repository`. `branch` was previously removed in favour of `pattern`. Similarly, `pattern` becomes computed and takes the value of `branch`. `branch` now becomes deprecated for removal in the next major release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! There were some other breaking changes that we may want to revert, specifically around changes to required_pull_request_reviews
dismissal_users
->dismissal_restrictions
dismissal_teams
->dismissal_restrictions
Additionally, dismissal_restrictions
is erroneously cited in the docs as dismissal_actors
(see #569).
Should we make dismissal_restrictions
a computed field w/ actor ID's from dismissal_users
and dismissal_teams
, adding a deprecation warning for those fields as well?
Type: schema.TypeString, | ||
Computed: true, | ||
Description: "GraphQL `node_id` of a repository", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
ForceNew: true, | ||
Description: "", | ||
Description: "`pattern` for a `BranchProtectionRule`", | ||
Deprecated: "use `pattern` after v4.0.0 instead", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
resource.TestCheckResourceAttr( | ||
"github_branch_protection.test", "branch", "main", | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
It would be great if the PR author could add in some state migrations so users that currently define |
Co-authored-by: Brandon Wood <[email protected]>
Closing this as we've discussed rolling forward with v4.0.0 instead. |
repository
was previously removed in favour ofrepository_id
. Werevert to
repository
here and transitionrepository_id
to a computedvalue that is derived from a GraphQL lookup against the provided
repository
.branch
was previously removed in favour ofpattern
. Similarly,pattern
becomes computed and takes the value ofbranch
.branch
now becomes deprecated for removal in the next major release.
Fixes https://github.com/terraform-providers/terraform-provider-github/issues/566