Add repository and organization rulesets

[o-sama]

Resolves #1777

---

Before the change?

Please see our docs on breaking changes to help!

• Yes
• No

---

This is also being worked on in #1807, seems we started working on it around the same time but came to different implementations and we can pair on this if preferred, @felixlut. Not 100% sure what the best way to handle this is 🙂

[felixlut]

@o-sama I'm game to collaborate, but also not sure how to do that most effectively 😅

I guess a good place to start would be to read each others PR:s and see which parts looks the best. And also agreeing on how the Terraform Schema should look.

Since you already have the org level stuff up and running, I think it's probably easier to base whatever changes we make on your PR.

I'll take a look at your code tomorrow after work (around 18 CET ish)

[o-sama]

@felixlut sounds good, feel free to make changes to the branch, I’ll add you as a collaborator. I’ll also add an example resource here in the morning so you can check it out.

I think your use of a utils file for the ruleset functions is smart, we can definitely extract most of the common logic out of the two resource helpers into that. If we end up needing to chat/call we can set up something to help us collaborate better 😄

[o-sama]

@felixlut Feel free to add me on discord, username is ih4sh, or any other messaging platform of your choice 🙂. we can chat about this or call there to avoid cluttering the comments. I’m in the EDT timezone but should be able to take my lunch when we talk about this.

[o-sama]

Made a change to use shared utils between repo and org rulesets, with a bool used to check for things specific to each, thanks for the idea @felixlut, picking the best from both of our changes 😄

Here’s an example ruleset which works on my end:

resource "github_repository_ruleset" "test" {
  name        = "test-fix"
  repository  = github_repository.repo.id
  target      = "branch"
  enforcement = "active"

  conditions {
    ref_name {
      include = ["~ALL"]
      exclude = []
    }
  }

  bypass_actors {
    actor_id    = 13473
    actor_type  = "Integration"
    bypass_mode = "always"
  }

  rules {
    creation = true

    update = true

    deletion                = true
    required_linear_history = true

    required_deployments {
      enabled                          = true
      required_deployment_environments = ["test"]
    }

    required_signatures = true

    pull_request {
      enabled                           = true
      required_approving_review_count   = 2
      required_review_thread_resolution = true
      require_code_owner_review         = true
      dismiss_stale_reviews_on_push     = true
      require_last_push_approval        = true
    }

    required_status_checks {
      required_check {
        context = “ci"
        integration_id = 15368
      }
      required_check {
        context = “x”
        integration_id = 15368
      }
      strict_required_status_checks_policy = true
    }

    non_fast_forward = true

    # branch_name_pattern {
    #   enabled  = true
    #   name     = "test"
    #   negate   = false
    #   operator = "starts_with"
    #   pattern  = "test"

    # }

  }
}

We should probably:

• Implement validation on whether or not the repo itself is within an org if it’s a repo ruleset, repo rulesets can contain metadata rules if the owner is an organization and not an individual.
• Add a data source for getting rulesets (Though this can probably be a different PR)
• Add a note in docs for required_status_checks being broken seemingly on the GitHub API side, not the client or provider, this is reinforced by the UI not being able to set up required status checks on my repo.
• Agree on our schema and check it thoroughly for both repos and orgs

[felixlut]

@o-sama I had no idea you could just create multiple blocks of the same field like you did below. IMO we should expose integration_id here as well, otherwise the contexts might just as well be a list of strings:

required_check {
  context = "ci"
}
required_check {
  context = ""
}

I think conditions can drop the ref_name, and be shortened down to below. Also, what do you think about adding separate bool values for the ~ALL and ~DEFAULT_BRANCH stuff?:

conditions {
  include = ["~ALL"]
  exclude = []
}

I think the data_sources can be a separate PR.

required_status_checks works in my PR. What crashes for you?

[felixlut]

Like I mentioned in #1808 (comment), I think we can drop the ref_name. It's just cluttering the usability imo.

[o-sama]

I think this one might be useful to keep in case it’s an org ruleset, those have include and exclude if you use repository_name, so it could be more clear to have ref_name there so the inner parameters aren’t at the top level and also in repository_name, but I have no strong preference here

[felixlut]

Fair point. Now that I've looked at the org level one a bit, I think it's fine to keep the ref_name there

[o-sama]

Sounds good, we can keep this in mind and then check with others once we’re ready to have this reviewed

[felixlut]

Sounds good to me

[felixlut]

I guess this is a feature not available trough the UI yet? I couldn't see it when I tested my solution out atleast

[o-sama]

This is available on org rulesets as well as repo rulesets for repos owned by an org, we should validate somewhere but it’d have to be outside of the schema’s validateFunc I think

[felixlut]

We could add a sanity check function, lets say validateRulesetResource(), which is called as the first step of all the CRUD operations. Something like I did here

func validatePortfolioResource(d *schema.ResourceData) error {
        switch selectionMode := d.Get("selection_mode"); selectionMode {
        case "NONE", "MANUAL", "REST":
                return nil


        case "TAGS":
                tags := d.Get("tags").([]interface{})
                if len(tags) == 0 {
                        return fmt.Errorf("validatePortfolioResource: When selection_mode is set to TAGS, you need atleast 1 tag, got: %+v", d.Get("tags"))
                }


                for _, tag := range d.Get("tags").([]interface{}) {
                        tagString := fmt.Sprint(tag)
                        if len(tagString) == 0 {
                                return fmt.Errorf("validatePortfolioResource: When selection_mode is set to TAGS, each tag must be non 0, got: %s", tagString)
                        }
                }
                return nil


        case "REGEXP":
                regexp := d.Get("regexp").(string)
                if len(regexp) == 0 {
                        return fmt.Errorf("validatePortfolioResource: When selection_mode is set to REGEXP, regexp must be set, got: \"%s\"", regexp)
                }
                return nil


        default:
                return fmt.Errorf("resourceSonarqubePortfolioCreate: selection_mode needs to be set to one of NONE, MANUAL, TAGS, REGEXP, REST")
        }
}

func resourceSonarqubePortfolioCreate(d *schema.ResourceData, m interface{}) error {
	if err := checkPortfolioSupport(m.(*ProviderConfiguration)); err != nil {
		return err
	}
       
        // ...
}

[felixlut]

I think this is a big nono in Terraform Providers. Automatically removing stuff from the state feels wrong. I think it should just crash, but I'm not sure what the best practices are here.

[o-sama]

This is something I took out of resource_repository, it’s essentially used for state refresh, if we can’t find it then it’s not there anymore and we update the state accordingly, at least that’s what I gathered from the current implementation of the provider

[felixlut]

Fair enough, I might be wrong on that point

[felixlut]

Add ValidateFunc

[felixlut]

Added in o-sama#1

[o-sama]

@kfcampbell How would I go about automated testing for enterprise features? I’m looking specifically at org-level rulesets and repo rulesets within an enterprise (metadata rules don’t work on regular orgs). I’ve tested manually and confirmed the functionality works.

[kfcampbell]

@o-sama I wish...I don't have easy answers here. From an automated CI perspective, we don't have integration tests in a healthy state. Would it be possible to create automated testing with enterprise info like this that I could then validate?

[o-sama]

@kfcampbell Sounds good, I’ll write something like that.

@felixlut I closed google/go-github#2836 since I got a response regarding the update rule, it should have the existing parameter but that only works for forked repos (but currently bugged anyways). So, I’ll add the parameter and retest that non-forked repos work, and it should be testable for forked repos once it’s fixed on the GitHub side. I wanted to push a new commit today but I’ve not been feeling well, so I’ll just add this stuff onto my next commit and do that for tomorrow/Wed.

[o-sama]

@kfcampbell I’ve added the enterprise tests here:

• terraform-provider-github/github/resource_github_repository_ruleset_test.go

  Line 118 in 33e8645

  t.Run("Creates and updates repository rulesets with enterprise features without errors", func(t *testing.T) {

• terraform-provider-github/github/resource_github_organization_ruleset_test.go

  Line 12 in 33e8645

  func TestGithubOrganizationRulesets(t *testing.T) {

Let me know if I need to fix anything or if threre’s something else needed on my end 🙂

@felixlut I’ve changed the import test to cover a ruleset with several rules enabled and added a field for the forked-repo-only param for update rule. I have a PR open in the go-github repo to fix the issue with nil-pointer dereference that this causes but the endpoint will still be bugged when using this param in the meantime so I included that in the docs. I think we’ve got everything covered now, once you’ve had a chance to review I think we’re good to mark this as ready for review as soon as we’ve got a new release of go-github. LMK what you think!

[kfcampbell]

I think we’re good to mark this as ready for review as soon as we’ve got a new release of go-github

@o-sama @felixlut great work here! I'm looking forward to that release.

[o-sama]

@kfcampbell @felixlut This should be good for review now. Updated the provider to use go-github v54 as well as the new resources. Although I noticed tests are not run on the CI check for this and other PRs. This PR requires enterprise tests to run so I’ll just defer to Keegan for those as discussed above 😄

[felixlut]

Read through the whole thing again now, and it lgtm.

[o-sama]

@kfcampbell This is now ready to be merged at your convenience provided the acceptance tests are successful when you run them, especially the enterprise ones since I can’t guarantee they’re good myself.

[kfcampbell]

Thank you both for all the effort here! The collaboration is wonderful to see. I'll get this merged now and released sometime next week!

[kingli-crypto]

Hi, thank you for implementing this.
Do we know when it will be released?

[csainty]

Super timely addition for me! Thank you.
But you've got a typo in one of the properties

https://registry.terraform.io/providers/integrations/github/latest/docs/resources/organization_ruleset#inlcude

"inlcude" instead of "include"

What's the compatibility-promise around fixing this? Do we need to now support both properties with one deprecated? Is it something you can correct in a patch release and just assume the usage is low?

[o-sama]

@csainty Since the release was cut only a few days ago, I think it's better that we cut a patch release to fix it, better to support only the correct spelling in the long run.

Also, apologies for that! Had to work on this a bunch during the nights between work and baby duties.

I just opened #1884 to fix this and I'm open to supporting both versions if that's the direction we want to go about it.

[csainty]

Awesome, thanks. And no need to apologise, a very easy typo to miss. I only spotted it when my config complained there was no include property defined even though I had "seen" it in the docs 😆

Some other feedback from my first tests with this.

• Some of the lists are limited to a single item. E.g. repository_id. But that looks like a mistake, even the read/import process will put multiple items in there. A locally built version dropping the MaxItems on definition looks to work, but I didn't have time yesterday to explore them all with tests etc.

• I am having trouble getting the import process to see my bypass_actor. I haven't been able to see what the problem is, or debug it though. Running with TF_LOG=DEBUG I see it in the JSON response from GitHub. All the names and structures look fine. But when the plan is written out it says it wants to add the one defined in the .tf and no mention of the one that is being read during import.

The config I am aiming to write looks like this

import {
  to = github_organization_ruleset.my_ruleset
  id = "123456"
}
resource "github_organization_ruleset" "my_ruleset" {
  name        = "My example ruleset"
  target      = "branch"
  enforcement = "active"
  bypass_actors {
    actor_type = "Integration"
    actor_id   = "123456"
  }
  conditions {
    repository_id = [
      123,
      456,
      789,
    ]
    ref_name {
      include = ["~DEFAULT_BRANCH"]
      exclude = []
    }
  }
  rules {
    deletion         = true
    non_fast_forward = true
    pull_request {
      required_approving_review_count = 1
      dismiss_stale_reviews_on_push   = true
      require_code_owner_review       = true
    }
  }
}

Basically I have a ruleset I selectively apply to some repositories where I want to give a GitHub App installation permission to bypass the rules.

When I plan (with the MaxItems edit inplace) I get

  # github_organization_ruleset.my_ruleset will be updated in-place
  # (imported from "123456")
  ~ resource "github_organization_ruleset" "my_ruleset" {
        enforcement = "active"
        etag        = "<snip>"
        id          = "123456"
        name        = "My example ruleset"
        node_id     = "<snip>"
        ruleset_id  = 123456
        target      = "branch"

      + bypass_actors {
          + actor_id   = 123456
          + actor_type = "Integration"
        }

        conditions {
            repository_id = [
                123,
                456,
                789,
            ]

            ref_name {
                exclude = []
                include = [
                    "~DEFAULT_BRANCH",
                ]
            }
        }

        rules {
            creation                = false
            deletion                = true
            non_fast_forward        = true
            required_linear_history = false
            required_signatures     = false
            update                  = false

            pull_request {
                dismiss_stale_reviews_on_push     = true
                require_code_owner_review         = true
                require_last_push_approval        = false
                required_approving_review_count   = 1
                required_review_thread_resolution = false
            }
        }
    }

Note the bypass_actors appearing as an add, causing the resource to think it needs editing.
As stated, I don't have a lead for you on why that is just yet. Not sure I'll get time today either, but I'll come back to you if I do further investigation

[o-sama]

@csainty thanks for the feedback man, much appreciated! There's only so much manual testing one can do and I don't have a test enterprise to run automated tests on.

I'll look into the stuff you raised and get to working on it tonight!