[MAINT] Fix `github_organization_ruleset` and `github_repository_ruleset` with `push` target #2958

deiga · 2025-12-02T20:12:55Z

Resolves #2929, #2467

Before the change?

The provider would crash from a 422 error response when following the example in the docs
Removing ref_name would cause the provider to Panic as ref_name is a required field
One could add rules which weren't valid for push rulesets

After the change?

The provider should correctly apply push rulesets to an organization
ref_name should no longer be needed to be set for push target
conditions & target validation logic should ensure correct fields are populated

Pull request checklist

~~Schema migrations have been created if needed (example)~~
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

github/resource_github_organization_ruleset.go

stevehipwell

I think this change likely wants to wait for the SDK upgrade as a lot of this area is modified in future versions.

FYI the error behaviours previously seen should have been mitigated by #2705 so if there is an error the provider should handle it gracefully.

github/resource_github_organization_ruleset.go

stevehipwell

Would it be possible to either de-scope this PR or open a new PR with the smallest number of changes possible to fix the outstanding bugs?

github/config.go

github/resource_github_organization_ruleset.go

stevehipwell · 2025-12-10T10:37:23Z

github/resource_github_organization_ruleset.go

 			},
 			"bypass_actors": {
-				Type:             schema.TypeList,
+				Type:             schema.TypeList, // TODO: These are returned from GH API sorted by actor_id, we might want to investigate if we want to include sorting


I think we should sort the returned values based on the inputs which would stop churn.

That sounds sensible, need to investigate where the best place for the sorting is

@stevehipwell after some investigation, it seems that schema.TypeSet would be the correct way to implement unsorted. What do you think?

deiga · 2025-12-10T13:55:16Z

@stevehipwell Yes, I agree. I've done that already in this PR: #2976

But I can't switch the base of this PR to point to that :)

Signed-off-by: Timo Sand <[email protected]>

… to org rulesets will never be a thing Signed-off-by: Timo Sand <[email protected]>

Signed-off-by: Timo Sand <[email protected]>

…re empty lists by default Signed-off-by: Timo Sand <[email protected]>

Signed-off-by: Timo Sand <[email protected]>

…_id` Signed-off-by: Timo Sand <[email protected]>

Signed-off-by: Timo Sand <[email protected]>

…s.context` is not empty Signed-off-by: Timo Sand <[email protected]>

Signed-off-by: Timo Sand <[email protected]>

This turns out to be failing as there is a bug in our implementation! Unit tests and fix coming up Signed-off-by: Timo Sand <[email protected]>

Signed-off-by: Timo Sand <[email protected]>

…esponse Signed-off-by: Timo Sand <[email protected]>

As they differ from `branch` and `tag` rules Signed-off-by: Timo Sand <[email protected]>

Signed-off-by: Timo Sand <[email protected]>

…ved` repos to be private This allows even EMU users to run these tests Signed-off-by: Timo Sand <[email protected]>

Signed-off-by: Timo Sand <[email protected]>

…` and `rules` Signed-off-by: Timo Sand <[email protected]>

Signed-off-by: Timo Sand <[email protected]>

…anything Signed-off-by: Timo Sand <[email protected]>

stevehipwell

I've added some review comments, mainly about the code structure.

stevehipwell · 2026-01-13T10:22:43Z

github/repository_rules_validation_utils.go

@@ -0,0 +1,157 @@
+package github


I'm not sure why we need this new file instead of using the util_rules.go file?

I felt that util_rules.go is already getting too long to reason about nicely. But if this is an anti-pattern I can move these there

github/repository_rules_validation_utils_test.go

stevehipwell · 2026-01-13T10:28:11Z

github/resource_github_organization_ruleset.go

+		CustomizeDiff: customdiff.All(
+			validateConditionsFieldBasedOnTarget,
+			validateOrganizationRulesetRules,
+		),


I don't think we should use separate function here as we're writing our own logic, that pattern is only really useful when we're composing. Could you take a look at how I've structured the schema and done this in #3069?

stevehipwell · 2026-01-13T10:31:30Z

github/resource_github_organization_ruleset.go

 	return []*schema.ResourceData{d}, nil
 }
+
+func validateConditionsFieldBasedOnTarget(ctx context.Context, d *schema.ResourceDiff, meta any) error {


If this is scoped to the organization ruleset then the name should reflect that, otherwise it should probably be in the util_rules.go file (idiomatically I'd expect this to be called util_ruleset.go).

stevehipwell · 2026-01-13T10:32:45Z

github/resource_github_organization_ruleset_test.go

+			}
+		`, resourceName, randomID)
+
+		check := resource.ComposeTestCheckFunc(


Can we please inline these instead of creating a separate var, this makes the code so much harder to read.

stevehipwell · 2026-01-13T10:34:08Z

github/resource_github_organization_ruleset_test.go

+			resource.TestCheckResourceAttr(
+				fmt.Sprintf("github_organization_ruleset.%s", resourceName),
+				"name",
+				fmt.Sprintf("test-push-%s", randomID),
+			),


Suggested change

resource.TestCheckResourceAttr(

fmt.Sprintf("github_organization_ruleset.%s", resourceName),

"name",

fmt.Sprintf("test-push-%s", randomID),

),

resource.TestCheckResourceAttr(fmt.Sprintf("github_organization_ruleset.%s", resourceName), "name", fmt.Sprintf("test-push-%s", randomID)),

Please can these be made a single line, it's the idiomatic Go pattern (no introducing line breaks) and makes the code much easier to read.

nickfloyd reviewed Dec 2, 2025

View reviewed changes

github/resource_github_organization_ruleset.go Outdated Show resolved Hide resolved

github/resource_github_organization_ruleset.go Outdated Show resolved Hide resolved

github/resource_github_organization_ruleset.go Show resolved Hide resolved

stevehipwell reviewed Dec 3, 2025

View reviewed changes

github/resource_github_organization_ruleset.go Outdated Show resolved Hide resolved

github/resource_github_organization_ruleset.go Outdated Show resolved Hide resolved

deiga force-pushed the org-ruleset-fix-push branch from 0531db8 to 9dd2965 Compare December 3, 2025 20:15

deiga changed the base branch from main to go-github-v68 December 3, 2025 20:15

deiga mentioned this pull request Dec 5, 2025

[BUG]: Removing bypass actors from Rulesets does not have any effect #2952

Closed

1 task

stevehipwell reviewed Dec 5, 2025

View reviewed changes

github/resource_github_organization_ruleset.go Outdated Show resolved Hide resolved

deiga force-pushed the org-ruleset-fix-push branch from 1bdfa45 to efd67ae Compare December 7, 2025 00:20

deiga changed the title ~~Fix github_organization_ruleset with push target~~ [MAINT] Fix github_organization_ruleset with push target Dec 7, 2025

deiga force-pushed the org-ruleset-fix-push branch from 52d95d4 to 6782aab Compare December 8, 2025 13:42

stevehipwell requested changes Dec 10, 2025

View reviewed changes

deiga changed the title ~~[MAINT] Fix github_organization_ruleset with push target~~ [MAINT] Fix github_organization_ruleset and github_repository_ruleset with push target Dec 17, 2025

deiga marked this pull request as ready for review December 19, 2025 06:35

deiga marked this pull request as draft January 6, 2026 08:06

deiga changed the base branch from go-github-v68 to main January 7, 2026 22:14

deiga force-pushed the org-ruleset-fix-push branch from 10baa83 to 1fe74fc Compare January 8, 2026 23:33

deiga added 14 commits January 10, 2026 08:31

Update descriptions and validations

db012e9

Signed-off-by: Timo Sand <[email protected]>

Add CustomizeDiff logic to validate on plan

ac73f34

Signed-off-by: Timo Sand <[email protected]>

Remove unused leftover

c323855

Signed-off-by: Timo Sand <[email protected]>

Add first validation test

ad57e88

Signed-off-by: Timo Sand <[email protected]>

Add validation error when conditions is missing

a993289

Signed-off-by: Timo Sand <[email protected]>

Add further validation tests

9167433

Signed-off-by: Timo Sand <[email protected]>

Remove unnecessary skip blocks as individual and anonymous access…

e859cf3

… to org rulesets will never be a thing Signed-off-by: Timo Sand <[email protected]>

Switch ref_name to Optional as push doesn't need a ref_name

4b63aec

Signed-off-by: Timo Sand <[email protected]>

Add Debug logging with tflog to validation

bacd7bb

Signed-off-by: Timo Sand <[email protected]>

Fix validation as ref_name, repository_name and repository_id a…

8daf742

…re empty lists by default Signed-off-by: Timo Sand <[email protected]>

Fix test output expectation

dfe82e6

Signed-off-by: Timo Sand <[email protected]>

Remove unnecessary panic test

27c832a

Signed-off-by: Timo Sand <[email protected]>

Improve validation output messages

d648b9e

Signed-off-by: Timo Sand <[email protected]>

Fix condition to require only one of repository_name or `repository…

1ee74cf

…_id` Signed-off-by: Timo Sand <[email protected]>

deiga added 22 commits January 10, 2026 08:31

Fix linter issues

b61e2eb

Signed-off-by: Timo Sand <[email protected]>

Add validation to ensure `rules.required_status_checks.required_check…

dd38c25

…s.context` is not empty Signed-off-by: Timo Sand <[email protected]>

Add test to ensure that required_checks is always required

102f716

Signed-off-by: Timo Sand <[email protected]>

Fix tests after rebase

821529a

Signed-off-by: Timo Sand <[email protected]>

Improve legibility of conditions description

8933a7f

Signed-off-by: Timo Sand <[email protected]>

Update descriptions

e2d4434

Signed-off-by: Timo Sand <[email protected]>

Add Acc test for push ruleset.

27b5959

This turns out to be failing as there is a bug in our implementation! Unit tests and fix coming up Signed-off-by: Timo Sand <[email protected]>

Add failing test for flattenConditions with no ref_name condition

fb2a513

Signed-off-by: Timo Sand <[email protected]>

Fix flattenConditions to work with push rulesets

a502c03

Signed-off-by: Timo Sand <[email protected]>

Add more tests for flattenConditions

f2dddbe

Signed-off-by: Timo Sand <[email protected]>

Enable debug logging in flattenConditions

730a26e

Signed-off-by: Timo Sand <[email protected]>

Ensures that flattenConditions returns an empty list on empty API r…

b7e2cfb

…esponse Signed-off-by: Timo Sand <[email protected]>

Add validation for push rules

fcf0acd

As they differ from `branch` and `tag` rules Signed-off-by: Timo Sand <[email protected]>

github_repository: Remove comment which doesn't hold true anymore

f6e8372

Signed-off-by: Timo Sand <[email protected]>

github_repository: Ensures that we don't try to PATCH an archived repo

4fc8fee

Signed-off-by: Timo Sand <[email protected]>

github_repository_ruleset: Change `TestGithubRepositoryRulesetArchi…

5b62488

…ved` repos to be private This allows even EMU users to run these tests Signed-off-by: Timo Sand <[email protected]>

Get TestAccGithubRepositoryRulesets to work

345a07b

Signed-off-by: Timo Sand <[email protected]>

repository_ruleset: Add tests for validations

3cd77d0

Signed-off-by: Timo Sand <[email protected]>

repository_ruleset: Implement validations for target, `conditions…

ac27d0e

…` and `rules` Signed-off-by: Timo Sand <[email protected]>

Updated ruleset docs

1a50eef

Signed-off-by: Timo Sand <[email protected]>

Extract validation functions to separate utils file with unit tests

a1a9a68

Signed-off-by: Timo Sand <[email protected]>

Fix ExpectError message

2a2277a

Signed-off-by: Timo Sand <[email protected]>

deiga force-pushed the org-ruleset-fix-push branch from 1fe74fc to 2a2277a Compare January 10, 2026 08:53

Fix push ruleset test config

fe27c4d

Signed-off-by: Timo Sand <[email protected]>

deiga marked this pull request as ready for review January 10, 2026 08:56

deiga requested review from nickfloyd and stevehipwell January 10, 2026 08:56

Remove repository target after thorough testing that it doesn't do …

c1019c3

…anything Signed-off-by: Timo Sand <[email protected]>

deiga mentioned this pull request Jan 11, 2026

|FEAT] Add ruleset rule for pull request required reviewers #3073

Draft

5 tasks

stevehipwell requested changes Jan 13, 2026

View reviewed changes

[MAINT] Fix github_organization_ruleset and github_repository_ruleset with push target #2958

Are you sure you want to change the base?

[MAINT] Fix github_organization_ruleset and github_repository_ruleset with push target #2958

Uh oh!

Conversation

deiga commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before the change?

After the change?

Pull request checklist

Does this introduce a breaking change?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevehipwell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevehipwell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deiga commented Dec 10, 2025

Uh oh!

stevehipwell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[MAINT] Fix `github_organization_ruleset` and `github_repository_ruleset` with `push` target #2958

[MAINT] Fix `github_organization_ruleset` and `github_repository_ruleset` with `push` target #2958

deiga commented Dec 2, 2025 •

edited

Loading