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

Add support for allow_update_branch and update provider min version #148

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

soerenmartius
Copy link
Member

@soerenmartius soerenmartius commented Feb 2, 2023

  • BREAKING CHANGE: Add support for allow_update_branch. Bump minimum supported version of the GitHub provider to v5.16
    as it contains a critical fix for branch protections.

@soerenmartius soerenmartius force-pushed the soerenmartius/allow_update_branch branch 2 times, most recently from 78d3197 to 35515a0 Compare February 3, 2023 11:23
@0x46616c6b
Copy link
Contributor

Is there a planned timeline to merge this and also have the branch protection updated to fix the deprecation? Would like to have both in the module to provide this features to my colleagues. Thanks a lot!

@soerenmartius soerenmartius marked this pull request as ready for review March 16, 2023 16:28
@soerenmartius soerenmartius requested review from mariux and a team as code owners March 16, 2023 16:28
@switchdk
Copy link

Hi @soerenmartius I don't know the module particularly well but thought I would try to provide some feedback. Thanks for updating.

In general, I noticed that branch_protection is referred to as branch_protection_v4. It should probably be updated to reflect the minimum required version is 5.16.

@@ -62,7 +62,7 @@ module "repository" {

required_status_checks = {
strict = true
contexts = ["ci/travis"]
checks = ["ci/travis"]

Choose a reason for hiding this comment

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

My understanding is that this uses branch_protection and not branch_protection_v3 so therefore contexts is still the correct term at least that is how I understand the documentation.

@@ -47,7 +48,7 @@ module "repository" {

admin_collaborators = ["terraform-test-user-1"]

branch_protections = [
branch_protections_v5 = [

Choose a reason for hiding this comment

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

I am not able to find a variable known as branch_protections_v5 in the code. I can find branch_protections_v4 and was wondering if this line is supposed to reference that variable instead? If so, I think we should rename all references to either branch_protections_v5 or just branch_protections.

@tobiasehlert
Copy link

@soerenmartius, want to look into updating the PR with the comments made from @switchdk?
I really want to have allow_update_branch 😎

@shinenelson
Copy link

Since this is going to be a breaking change, can we also please obsolete the defaults variable and clean the code up?

I could create a pull request for it targeting this branch if that is okay with the maintainers.

@soerenmartius soerenmartius force-pushed the soerenmartius/allow_update_branch branch from 35515a0 to 47f6413 Compare May 4, 2023 16:50
@soerenmartius soerenmartius force-pushed the soerenmartius/allow_update_branch branch from 47f6413 to f39f6e7 Compare May 4, 2023 16:52
@soerenmartius
Copy link
Member Author

branch_protections_v5

yeah that's a fair point - I wanted to do this since ages but as we're currently focussing on https://github.com/mineiros-io/terramate I really didn't find the time yet

@soerenmartius
Copy link
Member Author

Since this is going to be a breaking change, can we also please obsolete the defaults variable and clean the code up?

I could create a pull request for it targeting this branch if that is okay with the maintainers.

Let me discuss with the team if we can invest some time in a major refactoring

@pesarkhobeee
Copy link

Hello dear @soerenmartius , any possible date to fix this issue?

@thrix
Copy link

thrix commented Apr 11, 2024

@soerenmartius hi, is the required refactoring something that could be written down, then maybe somebody could contribute it :)

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.

None yet

7 participants