Fix ruleset bypass actors diff issues

[o-sama]

Resolves #1952 (Partial)

---

Before the change?

• When updating a ruleset you can get the following:

# github_repository_ruleset.test2 will be updated in-place
  ~ resource "github_repository_ruleset" "test2" {
        id          = "xxxxx"
        name        = "test-12345"
        # (6 unchanged attributes hidden)

      ~ bypass_actors {
          ~ actor_id    = 5 -> 1
          ~ actor_type  = "RepositoryRole" -> "OrganizationAdmin"
            # (1 unchanged attribute hidden)
        }
      ~ bypass_actors {
          ~ actor_id    = 1 -> 5
          ~ actor_type  = "OrganizationAdmin" -> "RepositoryRole"
            # (1 unchanged attribute hidden)
        }

        # (1 unchanged block hidden)
    }

I've seen this happen when the order in which bypass_actor blocks happen to be defined changes. Whether this is done manually or through an automation (I've noticed this happen with something I'm working on which uses a yaml file for defining things).

A while back I made a small change which tries to fix this by sorting when flattening the block, but that doesn't cover this case (and potentially others) and isn't technically the way to do it.

After the change?

• Now we have a DiffSuppressFunc defined which checks every bypass actor and makes sure a diff only shows up when something does change. The caveat here is it's done several times because each key is passed to the function once. Personally though I would still take this over having a diff every time TF runs, and logically it doesn't make sense to have many bypass actors on a ruleset, so we shouldn't expect a real impact on performance.

The following is the same change as above:

github_repository_ruleset.test2: Refreshing state... [id=xxxxx]
github_organization_ruleset.test: Refreshing state... [id=xxxxx]

No changes. Your infrastructure matches the configuration.

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)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

• Yes
• No

---

[o-sama]

I looked into the inability to remove bypass actors by removing the block(s) and it doesn't seem like something that's fixed through the provider's implementation. Please correct me if my thinking below is wrong.

bypass actors is a list of objects, each is a block in TF, this create a slice of type []*BypassActor. in Go, the zero value of a slice is nil, and so, if we create an empty slice or pass nil we get the same behaviour (we create an empty slice in this case).

go-github uses omitempty when serializing the struct into JSON, and automatically omits the empty slice of bypass actors. I don't have visibility into the GitHub API implementation but It seems like when you send a request without bypass actors as a field, it doesn't actually remove them. If we were to send a request with an empty list it should, however, remove them (I think).

E.g. I looked into the debug logs and this is the ruleset in the request sent from the provider to the API:

{
  "name": "test-123",
  "target": "branch",
  "source_type": "Organization",
  "source": "XXXXXXXXXX",
  "enforcement": "active",
  "conditions": {
   "ref_name": {
    "include": [],
    "exclude": []
   },
   "repository_id": {
    "repository_ids": [
     XXXXXXXXXXX,
     XXXXXXXXXXX
    ]
   }
  },
  "rules": [
   {
    "type": "creation"
   },
   {
    "type": "branch_name_pattern",
    "parameters": {
     "name": "test",
     "negate": false,
     "operator": "starts_with",
     "pattern": "test"
    }
   }
  ]
 }

I will test out my theory and if needed, make a PR to go-github to get this addressed in the next release of the client.

[kfcampbell]

I will test out my theory and if needed, make a PR to go-github to get this addressed in the next release of the client.

That sounds great, thank you @o-sama! We appreciate all your contributions.

[felixlut]

I randomly stumbled upon this issue myself last week in jdamata/terraform-provider-sonarqube#211.

I started out with defining the selected_projects attribute as a schema.TypeList. While testing the provider I noticed that terraform plan always tried to alter the ordering of the projects, which is just noise. I tried all kinds of weird logic sorting the lists and whatnot, but those where all quite a bit flaky, and just felt too unintuitive. The way I finally solved it was to use a schema.TypeSet instead. From the Terraform documentation the latter is meant to be used when the order matters, while the former is unordered. This solved the issue because Terraform in the plan stage simply didn't care about the ordering of the underlying data, just that they included the same objects.

I'm not sure what implications there would be in switching to the set now that schema.TypeList is out in the wild already (the internal state representation seems to be different from the docs at the very least), but it might be valuable information to know in the future.

From most of the Terraform providers I've looked into so far (including this one), it seems that schema.TypeSet is highly underused in favor of schema.TypeList, especially for nested attributes. I might be overlooking a reason not to use it, but I think an unordered set usually is a more accurate representation of the underlying data than an ordered list.

[o-sama]

I remember when we worked on this initially I saw TypeList used elsewhere and so I used it for most of the nested schemas.

Similarly I wasn't aware this would be an issue to be honest and so it seems like it would be fine.

In hindsight it'd have been better to use TypeSet, but like you pointed out it has potential implications if we change it, even with a state migration function it can still be a breaking change and so a custom diff function is the less intrusive way of avoiding a diff every time this is run.

I am interested though in why it is underused compared to TypeList, although things should be better as the provider is slowly migrated away from the old sdk.

[felixlut]

Ye I'm in the same camp. I'd never used the set for a nested schema before last week, and I'm confused as to why it's not used more. Maybe I'll find out in a month when I curse myself for using it in the SonarQube provider 😅

[kfcampbell]

@o-sama where did you land on the go-github PR? What does that mean for this PR? I'd like to check just to make sure this doesn't get forgetten.

[stevehipwell]

What needs to happen before we can get this addressed? We're currently seeing a constant churn on both the github_organization_ruleset & github_repository_ruleset.

[o-sama]

@kfcampbell I was tied up between work and a different feature in go-github and haven't implemented the change I mentioned yet. I've worked on it locally but I just have to add test cases and push my changes for review.

I think the PR as is should fix the diff issue but it wouldn't add the ability to remove bypass actors since as far as I'm aware the client lacks that ability still.

I've been abroad for the last week or so but should have some time to work on that in the next couple of days. My bad on the delay!

[usmonster]

Thanks for your effort on this, @o-sama! Please let me know if there's anything you need help with. 🙏

[o-sama]

@kfcampbell & @nickfloyd Apologies for the delay here, It's been a hectic couple of months for me. I'm going to be creating the PR on go-github in the next couple of hours, but in the meantime is it possible for us to get this out and possibly get a release for it when that's appropriate? I think this will still help a lot of folks out with the perpetual diff they're seeing in their rulesets which costs a lot of time since each ruleset edit is at least 1 second with the write delay to account for rate limits.

I'll have to revisit my notes but IIRC once I get the go-github PR merged and released, it'll simply be a matter of updating the client's version and the issue with removing bypass actors would be fixed, so it'd be more of a general update than a code change specific to rulesets.

[kfcampbell]

@o-sama Sounds good! When that's merged, we can also clean up the diff suppress functions introduced here.

[o-sama]

@kfcampbell The diff suppress func will likely need to stay since this issue comes from the default diff func comparing the objects as a string one line at a time, so you'd likely always have a diff. I'll try changing that to a set at some point and having a state migration func to handle the change without needing anything on the users' end, how does that sound?

By the way thank you for the release! Always appreciate your timely responses!

[kfcampbell]

Ahh, thanks for clarifying. That sounds good to me! I'm grateful for your contributions to this project.