Updating resource_gitlab_branch_protection to support the full API.

[jasoncarlsen]

No description provided.

[sfang97]

This LGTM-- @roidelapluie could you give this an additional review?

[armsnyder]

@jasoncarlsen Can you rebase and fix the conflict with #359 where the code_owner_approval_required attribute was added already?

[jasoncarlsen]

Yeah I can do this. I'll see if I can't carve out some time tomorrow to take care of it.

[jasoncarlsen]

Done. Let me know if it looks alright.

[armsnyder]

Still some cleanup to do after the rebase it looks like.

Can you also test the push_access_levels, merge_access_levels, and unprotect_access_levels attributes?

[armsnyder]

For these arrays with MaxSize 1, can you change their schemas to match the API please? (For example user_id should be an int, not a list.)

[jasoncarlsen]

This one is a bit complicated. Maybe things have changed in the intervening months but back when I wrote this, the Terraform SDK didn't support...something (trying to recall, it's been so long...ah here it is, for TypeMap: "Using the Elem block to define specific keys for the map is currently not possible."). After searching around, the work-around that folks were using was a list of max 1 with a custom element that was a map[type of key].

So the maps are type string since "user_id", "group_id", and "access_level" are all strings that denote what kind of values are coming, and the values themselves are lists with types that should match the GitLab API. The values of "user_id" and "group_id" are set to a list of type int and the "access_level" is set to a list of type string.

In a Terraform config, the resource would thus look like this:

resource "gitlab_branch_protection" "branch_protect" {
	project = gitlab_project.foo.id
	branch = "branch_foo"
	allowed_to_push {
                group_id = [45, 101, 453]
		access_level = ["maintainer"]
	}
	allowed_to_merge {
                user_id = [1, 2, 3]
                access_level = ["maintainer", "developer"]
    }
	allowed_to_unprotect {
		access_level = ["developer"]
	}
}

[armsnyder]

Thanks, so it is about enforcing fields at the schema level. This might be the thread you were looking at.

My only concern is if GitLab modifies the API in a way that is legal in terms of the raw JSON, but breaks this translation here. For example, they could add a new field to the ProtectBranchPermissionOptions object that isn't mutually exclusive with the others. This would force us to make a breaking change to the provider.

Would this work? Forgive me if I'm still not understanding the limitation. 😄

1. Remove MaxItems: 1
2. Change "user_id" Type to int
3. Change "group_id" Type to string
4. Change "access_level" Type to string
5. Move your ExactlyOneOf check into the inner schema with these attributes.

This is the AWS provider example that was mentioned in that other issue. Linking it as a reference.

[jasoncarlsen]

Yeah that thread is the one I looked at; apparentlymart's first response is very familiar. Is this what you were thinking?

resource "gitlab_branch_protection" "branch_protect" {
	project = gitlab_project.foo.id
	branch = "branch_foo"
	allowed_to_push {
                {
                        group_id = 45
                }
                {
                        group_id = 101
                }
                {
                        group_id = 453
                }
                {
               		access_level = "maintainer"
                }
	}
	allowed_to_merge {
                {
                        user_id = 1
                }
                {
                        user_id = 2
                }
                {
                        user_id = 3
                }
                {
                        access_level = "developer"
                }
                {
                        access_level = "maintainer"
                }
        }
	allowed_to_unprotect {
		access_level = "developer"
	}
}

Basically each of the allowed_to_* attributes can have multiple maps as values, and each map can only specify one value for one permission type. If you were thinking of something else, please let me know. This would work but it's very messy...a single list makes far more sense in my opinion...I mean just look at these two examples and tell me which one you'd rather maintain :)

I do see your point regarding the design principles though. GitLab's API uses a list of single element maps whereas I flipped it around here and made it a map of lists. I feel like this is a pretty minor change and the improvement in resource clarity justifies the deviation. Even if GitLab changes the API, we'd still have to update the xanzy/go-gitlab library and that in turn would require changes to this resource. The mapping between resource attributes to the underlying go-gitlab struct is already encapsulated in "constructProtectBranchPermissionOptions".

[armsnyder]

Yeah I see your point. This is what I'm proposing actually:

resource "gitlab_branch_protection" "branch_protect" {
  project = "project_foo"
  branch = "branch_foo"
  allowed_to_push {
    group_id = 45
  }
  allowed_to_push {
    group_id = 101
  }
  allowed_to_push {
    access_level = "maintainer"
  }
  allowed_to_push {
    user_id = 1
  }
  allowed_to_push {
    user_id = 2
  }
  allowed_to_unprotect {
    access_level = "developer"
  }
  allowed_to_unprotect {
    user_id = 2
  }
}

That's taking what you have, but changing the inner lists to their element types, and removing the MaxItems: 1 limit from the blocks.

I agree that as a user of this provider, if I had a list of user IDs, I would rather write:

locals {
  user_ids = [1, 2, 3]
}

resource "gitlab_branch_protection" "branch_protect" {
  project = "project_foo"
  branch = "branch_foo"
  allowed_to_push {
    user_ids = local.user_ids
  }
}

Than:

locals {
  user_ids = [1, 2, 3]
}

resource "gitlab_branch_protection" "branch_protect" {
  project = "project_foo"
  branch = "branch_foo"
  dynamic "allowed_to_push" {
    for_each = local.user_ids
    content {
      user_id = allowed_to_push.value
    }
  }
}

That said.....

Even if GitLab changes the API, we'd still have to update the xanzy/go-gitlab library and that in turn would require changes to this resource.

Yes, but they would be backward-compatible changes. My concern is if GitLab added a new field that interacted somehow with user_id, for example, there would be no way to add it to the current schema design without making a backward-incompatible change or some kind of messy workaround.

GitLab could have designed their API using the simpler schema you chose, but they didn't. If they want the flexibility of adding new fields, then we should mirror that flexibility in the provider. It's unfortunate when you have to weigh usability and maintainability, and I don't like it either, but in this case I'd still ultimately side with following the API design.

[armsnyder]

We can see if @nicholasklick or @roidelapluie wants to add any opinions. 😄 It's a tough design call to make.

[roidelapluie]

We should use a list of single element maps . Terraform should be as close as the API as possible

[sirlatrom]

@jasoncarlsen Will you have the opportunity to proceed with this PR, or would it be alright if I carry it in a new PR?

[jasoncarlsen]

@sirlatrom Unfortunately I won't have any time in the near future. If you want to carry it to a new PR, then please go for it.

[sirlatrom]

@sirlatrom Unfortunately I won't have any time in the near future. If you want to carry it to a new PR, then please go for it.

My PR for this is ready for review: #556

[grv87]

@jasoncarlsen, please note that @sirlatrom in xanzy/go-gitlab#1066 renamed ProtectBranchPermissionOptions (back) to BranchPermissionOptions.
This MR can't be compiled with the recent go-gitlab library

[grv87]

Shouldn't it be TypeSet?

[zunkree]

We badly need that feature so how can I help to make that PR merged?

[mattkasa]

@jasoncarlsen thank you so much for working on this 💯

I'm going to go ahead and close this PR as it's been moved ahead in #556 👍