-
Notifications
You must be signed in to change notification settings - Fork 762
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
Feature: branch protection conversation resolution #904
Feature: branch protection conversation resolution #904
Conversation
I would like this - how difficult is the conflict resolution? Can I help move this along? |
93e41a0
to
2cc591a
Compare
Hi @bleggett, thanks for the prompt. I've just rebased the code. |
Hi @jcudit, just wondering what the process is around getting a PR reviewed? Thanks |
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.
Looking like a promising contribution so far with the following caveats:
- Can we get some clarity on the v4 dependency update?
- Can we drop the edits to the
_v3
resource? It has been in a frozen state since released so that we do not diverge from the recommended resource.
@@ -10,7 +10,7 @@ require ( | |||
github.com/hashicorp/hcl/v2 v2.3.0 // indirect | |||
github.com/hashicorp/terraform-config-inspect v0.0.0-20191212124732-c6ae6269b9d7 // indirect | |||
github.com/hashicorp/terraform-plugin-sdk v1.7.0 | |||
github.com/shurcooL/githubv4 v0.0.0-20201206200315-234843c633fa | |||
github.com/shurcooL/githubv4 v0.0.0-20210725200734-83ba7b4c9228 |
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.
Can we get some insight into why this dependency is changing? Not a blocker, but hoping to understand what we're currently missing.
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.
@jcudit The structs/variables for Conversation Resolution do not exist with the older version of githubv4
.
@jcudit There are existing issues relating to performance of the branch protection resource using GraphQL. Therefore, I've implemented the configuration in both resources so that people have the option of choosing the more performant resource. Personally, I can't use the How did you want to proceed? |
I think we can ship to both resources in this case, but overall discourage adding more to the |
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.
Tests well, thanks for the contribution!
Sorry but quick question. The docs state that this new Yet, after updating the provider our plans break:
Is that an issue or wanted behaviour? |
* feat: add conversation resolution variable on branch protection v3 * chore: update githubv4 package * feat: add conversation resolution variable on branch protection
This PR addresses #868 and updates the following resources:
You may find it easier to review commit by commit.