bugfix: include empty Checks array on v3 branch protection creation #1409

TheQueenIsDead · 2022-12-04T23:19:11Z

Resolves #1307 #1147

Behavior

Before the change?

Branch protections can not be created due to a nil error on the upstream Github API.

After the change?

The nil value is now passed as an empty array which causes the branch protection to be applied successfully.

Other information

Aims to resolve this issue Cannot create github_branch_protection_v3 after update to v5.3.0 #1307 by implementing the upstream suggestion for resolution as proposed here Error when creating branch protection google/go-github#2467 (comment)

Additional info

Pull request checklist

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

Yes (Please add the Type: Breaking change label)
No

If Yes, what's the impact:

N/A

Pull request type

Please add the corresponding label for change this PR introduces:

Bugfix: Type: Bug
Feature/model/API additions: Type: Feature
Updates to docs or samples: Type: Documentation
Dependencies/code cleanup: Type: Maintenance

…o mitigate upstream bug

TheQueenIsDead · 2022-12-04T23:24:19Z

I don't seem to be able to add a label to it, but this would be a Type: Bug PR 👍

kfcampbell · 2022-12-06T00:17:40Z

@TheQueenIsDead this approach sounds reasonable to me! Please let me know if you have any issues testing this. I'm looking forward to getting it merged!

TheQueenIsDead · 2022-12-06T22:45:41Z

I setup a test file like so for the following repository https://github.com/TheDeadPoetSociety/terraform-template-module:

resource "github_branch_protection_v3" "bp" {

  repository = "terraform-template-module"
  branch     = "master"


  enforce_admins = true
  required_pull_request_reviews {
    dismiss_stale_reviews = true
  }
  required_status_checks  {
    strict = false
    contexts = []
  }
}

terraform {
  required_providers {
    github = {
      source = "integrations/github"
    }
  }
}

provider "github" {
  # Configuration options
  owner = "TheDeadPoetSociety"
  token = "github_pat_XYZ"
}

When I ran the following I received the expected error:

$ go build -o ~/go/bin/
$ terraform init -upgrade

Initializing the backend...

Initializing provider plugins...
- Finding latest version of integrations/github...
- Using previously-installed integrations/github v5.11.0

Terraform has been successfully initialized!
...

$ terraform apply

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # github_branch_protection_v3.bp will be created
  + resource "github_branch_protection_v3" "bp" {
      + branch                          = "master"
      + enforce_admins                  = true
      + etag                            = (known after apply)
      + id                              = (known after apply)
      + repository                      = "terraform-template-module"
      + require_conversation_resolution = false
      + require_signed_commits          = false

      + required_pull_request_reviews {
          + dismiss_stale_reviews           = true
          + required_approving_review_count = 1
        }

      + required_status_checks {
          + strict = false
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

github_branch_protection_v3.bp: Creating...
╷
│ Error: PUT https://api.github.com/repos/TheDeadPoetSociety/terraform-template-module/branches/master/protection: 422 Invalid request.
│ 
│ No subschema in "anyOf" matched.
│ For 'properties/checks', nil is not an array.
│ Not all subschemas of "allOf" matched.
│ For 'anyOf/1', {"strict"=>false, "checks"=>nil} is not a null. []
│ 
│   with github_branch_protection_v3.bp,
│   on main.tf line 4, in resource "github_branch_protection_v3" "bp":
│    4: resource "github_branch_protection_v3" "bp" {
│ 
╵

When I ran a plan with my local version, the apply worked:

$ go build -o ~/go/bin/
$ TF_CLI_CONFIG_FILE=/home/david/GolandProjects/terraform-provider-github/examples/dev.tfrc terraform apply
╷
│ Warning: Provider development overrides are in effect
│ 
│ The following provider development overrides are set in the CLI configuration:
│  - integrations/github in /home/david/go/bin
│ 
│ The behavior may therefore not match any released version of the provider and applying changes may cause the state to become incompatible with published releases.
╵

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # github_branch_protection_v3.bp will be created
  + resource "github_branch_protection_v3" "bp" {
      + branch                          = "master"
      + enforce_admins                  = true
      + etag                            = (known after apply)
      + id                              = (known after apply)
      + repository                      = "terraform-template-module"
      + require_conversation_resolution = false
      + require_signed_commits          = false

      + required_pull_request_reviews {
          + dismiss_stale_reviews           = true
          + required_approving_review_count = 1
        }

      + required_status_checks {
          + strict = false
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

github_branch_protection_v3.bp: Creating...
github_branch_protection_v3.bp: Creation complete after 4s [id=terraform-template-module:master]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

Cool to see!
The resulting protection in Github looks like this:

I also tested creating the resource with the latest provider and updating the required_status_checks with the local provider thereafter

$ go build -o ~/go/bin/
$ terraform apply

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # github_branch_protection_v3.bp will be created
  + resource "github_branch_protection_v3" "bp" {
      + branch                          = "master"
      + enforce_admins                  = true
      + etag                            = (known after apply)
      + id                              = (known after apply)
      + repository                      = "terraform-template-module"
      + require_conversation_resolution = false
      + require_signed_commits          = false

      + required_pull_request_reviews {
          + dismiss_stale_reviews           = true
          + required_approving_review_count = 1
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

github_branch_protection_v3.bp: Creating...
github_branch_protection_v3.bp: Creation complete after 4s [id=terraform-template-module:master]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
$ # Uncomment required_status_checks
$ TF_CLI_CONFIG_FILE=/home/david/GolandProjects/terraform-provider-github/examples/dev.tfrc terraform apply
╷
│ Warning: Provider development overrides are in effect
│ 
│ The following provider development overrides are set in the CLI configuration:
│  - integrations/github in /home/david/go/bin
│ 
│ The behavior may therefore not match any released version of the provider and applying changes may cause the state to become incompatible with published releases.
╵
github_branch_protection_v3.bp: Refreshing state... [id=terraform-template-module:master]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # github_branch_protection_v3.bp will be updated in-place
  ~ resource "github_branch_protection_v3" "bp" {
        id                              = "terraform-template-module:master"
        # (6 unchanged attributes hidden)

      + required_status_checks {
          + strict = false
        }

        # (1 unchanged block hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

github_branch_protection_v3.bp: Modifying... [id=terraform-template-module:master]
github_branch_protection_v3.bp: Modifications complete after 4s [id=terraform-template-module:master]

Apply complete! Resources: 0 added, 1 changed, 0 destroyed.

I also tested creating branch protections without specifying contexts at all, and adding strict = true to pre-existing protections. Worked well without any mention of contexts 👍

TheQueenIsDead · 2022-12-06T22:58:26Z

Ah, it does fall flat when an apply is made with the new provider and the checks array is not null :-(

$ terraform apply

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # github_branch_protection_v3.bp will be created
  + resource "github_branch_protection_v3" "bp" {
      + branch                          = "master"
      + enforce_admins                  = true
      + etag                            = (known after apply)
      + id                              = (known after apply)
      + repository                      = "terraform-template-module"
      + require_conversation_resolution = false
      + require_signed_commits          = false

      + required_pull_request_reviews {
          + dismiss_stale_reviews           = true
          + required_approving_review_count = 1
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

github_branch_protection_v3.bp: Creating...
github_branch_protection_v3.bp: Creation complete after 3s [id=terraform-template-module:master]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

$ TF_CLI_CONFIG_FILE=/home/david/GolandProjects/terraform-provider-github/examples/dev.tfrc terraform apply
╷
│ Warning: Provider development overrides are in effect
│ 
│ The following provider development overrides are set in the CLI configuration:
│  - integrations/github in /home/david/go/bin
│ 
│ The behavior may therefore not match any released version of the provider and applying changes may cause the state to become incompatible with published releases.
╵
github_branch_protection_v3.bp: Refreshing state... [id=terraform-template-module:master]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # github_branch_protection_v3.bp will be updated in-place
  ~ resource "github_branch_protection_v3" "bp" {
        id                              = "terraform-template-module:master"
        # (6 unchanged attributes hidden)

      + required_status_checks {
          + contexts = [
              + "example",
            ]
          + strict   = true
        }

        # (1 unchanged block hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

github_branch_protection_v3.bp: Modifying... [id=terraform-template-module:master]
╷
│ Error: PUT https://api.github.com/repos/TheDeadPoetSociety/terraform-template-module/branches/master/protection: 422 Invalid request.
│ 
│ No subschema in "anyOf" matched.
│ More than one subschema in "oneOf" matched.
│ Not all subschemas of "allOf" matched.
│ For 'anyOf/1', {"strict"=>true, "contexts"=>["example"], "checks"=>[]} is not a null. []
│ 
│   with github_branch_protection_v3.bp,
│   on main.tf line 4, in resource "github_branch_protection_v3" "bp":
│    4: resource "github_branch_protection_v3" "bp" {
│ 
╵

TheQueenIsDead · 2022-12-06T23:09:53Z

I am starting to realise that Luis was quite correct in his upstream comment about contexts:
google/go-github#2467 (comment)

TheQueenIsDead · 2022-12-07T01:03:46Z

I'm starting to understand that the issue seems to be that both Contexts and Checks can not be defined. The issue is that we can not fully ommit Checks given the removal of the omitempty flag, it always defaults to an empty array, even if the client specifies nil:
https://github.com/integrations/terraform-provider-github/blob/main/vendor/github.com/google/go-github/v48/github/repos.go#L985

type RequiredStatusChecks struct {
	// Require branches to be up to date before merging. (Required.)
	Strict bool `json:"strict"`
	// The list of status checks to require in order to merge into this
	// branch. (Deprecated. Note: only one of Contexts/Checks can be populated,
	// but at least one must be populated).
	Contexts []string `json:"contexts,omitempty"`
	// The list of status checks to require in order to merge into this
	// branch.
	Checks []*RequiredStatusCheck `json:"checks"`
}

I thought I was being smart with the empty array, but the existence of both keys is the issue :-(

@kfcampbell , I have had success with using the provided "contexts" array and creating Checks instead, but this seems a bit hacky, as an example:

func expandRequiredStatusChecks(d *schema.ResourceData) (*github.RequiredStatusChecks, error) {
	if v, ok := d.GetOk("required_status_checks"); ok {
		vL := v.([]interface{})
		if len(vL) > 1 {
			return nil, errors.New("cannot specify required_status_checks more than one time")
		}
		rsc := new(github.RequiredStatusChecks)

		for _, v := range vL {
			// List can only have one item, safe to early return here
			if v == nil {
				return nil, nil
			}
			m := v.(map[string]interface{})
			rsc.Strict = m["strict"].(bool)

			contexts := expandNestedSet(m, "contexts")
                        // Iterate the provided "contexts" but pass them to the SDK as contexts with default app_id's
			rsc.Checks = func() []*github.RequiredStatusCheck {
				var checks []*github.RequiredStatusCheck
				for _, c := range contexts {
					checks = append(checks, &github.RequiredStatusCheck{
						Context: c,
						AppID:   nil,
					})
				}
				return checks
			}()
		}
		return rsc, nil
	}

	return nil, nil
}

Would the best solution be to implement full support for contexts like how k24dizzle was going to do before he got blocked on the upstream provider? (This PR for reference: #1051)

If so I will close this PR for the moment and perhaps see if I can take-over where they left off 👍

TheQueenIsDead · 2022-12-07T22:19:11Z

This workaround was worth trying, but I think the best thing to do would be to support checks.

I have opened up a WIP PR to implement this here: #1415

bugfix: include empty Checks array on v3 branch protection creation t…

5ed8dc1

…o mitigate upstream bug

TheQueenIsDead mentioned this pull request Dec 4, 2022

Cannot create github_branch_protection_v3 after update to v5.3.0 #1307

Closed

Merge branch 'main' into bugfix/1307-branch-protection-v3-creation

9a6b2c0

kfcampbell added the Type: Bug Something isn't working as documented label Dec 6, 2022

TheQueenIsDead closed this Dec 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix: include empty Checks array on v3 branch protection creation #1409

bugfix: include empty Checks array on v3 branch protection creation #1409

TheQueenIsDead commented Dec 4, 2022 •

edited

TheQueenIsDead commented Dec 4, 2022

kfcampbell commented Dec 6, 2022

TheQueenIsDead commented Dec 6, 2022

TheQueenIsDead commented Dec 6, 2022

TheQueenIsDead commented Dec 6, 2022

TheQueenIsDead commented Dec 7, 2022

TheQueenIsDead commented Dec 7, 2022

bugfix: include empty Checks array on v3 branch protection creation #1409

bugfix: include empty Checks array on v3 branch protection creation #1409

Conversation

TheQueenIsDead commented Dec 4, 2022 • edited

Behavior

Before the change?

After the change?

Other information

Additional info

Pull request checklist

Does this introduce a breaking change?

Pull request type

TheQueenIsDead commented Dec 4, 2022

kfcampbell commented Dec 6, 2022

TheQueenIsDead commented Dec 6, 2022

TheQueenIsDead commented Dec 6, 2022

TheQueenIsDead commented Dec 6, 2022

TheQueenIsDead commented Dec 7, 2022

TheQueenIsDead commented Dec 7, 2022

TheQueenIsDead commented Dec 4, 2022 •

edited