[MAINT] Fix `github_organization_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

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

nickfloyd · 2025-12-02T21:33:23Z

github/resource_github_organization_ruleset.go

 						"ref_name": {
 							Type:     schema.TypeList,
-							Required: true,
+							Optional: true,


This is a non breaking change ➕

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.

stevehipwell · 2025-12-03T14:56:12Z

github/resource_github_organization_ruleset.go

+				ValidateFunc: validation.StringInSlice([]string{"branch", "tag", "push", "repository"}, false),
+				Description:  "The target of the ruleset. Possible values are `branch`, `tag`, `push` and `repository`.",


The SDK doesn't support repository target rules, mainly because I've only just discovered that despite being in a separate UI area they're merged into the existing API. This is made even less discoverable by the top example of targets not having repository as an option, the schemas not being updated, and nobody at GitHub being able to give me a definitive answer.

v79 of the SDK does support those: https://github.com/google/go-github/blob/master/github/rules.go#L220

I honestly wanted to already put them there, so that it's ready when we get to upgrade. But I can remove it for now

I literally just added support for them in google/go-github#3850 and they've just been released in v80.0.0.

This needs removing.

stevehipwell · 2025-12-03T15:02:59Z

github/resource_github_organization_ruleset.go

 				Optional:    true,
 				MaxItems:    1,
-				Description: "Parameters for an organization ruleset condition. `ref_name` is required alongside one of `repository_name` or `repository_id`.",
+				Description: "Parameters for an organization ruleset condition. For `branch` and `tag` targets, `ref_name` is required alongside one of `repository_name` or `repository_id`. The `push` target does not require `ref_name` conditions. For `repository` target, the conditions object should only contain the `repository_name` and the `repository_id`.",


This doesn't look quite right, all org rulesets support complex repository targeting (SDK dependent). Push and repository (see above) targets don't support a ref target.

I'm not sure what you mean, I extrapolated this based on the REST API description. AFAIK there isn't anything more we can currently use (since custom_property targeting is not an option yet)
Though it's true that ref_name is optional even for tag and branch targets

The wording isn't right even if the field names are, the REST API makes a complete mess of this too. My point was that it'd be worth making the comment clearer as to what the fields actually do.

You are right that we can try to describe it better than the API docs. I'll try my hand at that.

stevehipwell · 2025-12-05T13:13:52Z

github/resource_github_organization_ruleset.go


 	var ruleset *github.RepositoryRuleset

 	ruleset, _, err = client.Organizations.UpdateRepositoryRuleset(ctx, org, rulesetID, *rulesetReq)


Checking if the bypass actors have changed and the length is 0 here would allow you to use the clear function to resolve #2952. See #2891 (comment).

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

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

…sport` Updated the logging transport in the RateLimitedHTTPClient function to use NewLoggingHTTPTransport. As the subsystem would need to be explicitly inititated in each resource Signed-off-by: Timo Sand <[email protected]>

By using the `*Context` CRUD functions we get context pass-through and enable HTTP Req/Resp Debug logging Signed-off-by: Timo Sand <[email protected]>

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

It's a required field in the API and go-github doesn't use `omitempty`, so it submits `nil` if it isn't sent explicitly. This change tries to keep it in the state, without having a configuration option for it (poor choice?) And it defaults to all 3 available merge methods if it can't set something from the state. Signed-off-by: Timo Sand <[email protected]>

This is a required field and the SDK doesn't omit if it's empty Signed-off-by: Timo Sand <[email protected]>

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

This enables better debugging during tests as we are able to use get logs for HTTP req/resp Signed-off-by: Timo Sand <[email protected]>

…on_rulesets_without_errors` to pass Signed-off-by: Timo Sand <[email protected]>

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

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

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

…_all_bypass_modes` Main fix: `bypass_actors` is returned as sorted from GH API so tests need re-indexing Signed-off-by: Timo Sand <[email protected]>

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]>

By using the `*Context` CRUD functions we get context pass-through and enable HTTP Req/Resp Debug logging 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]>

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?

stevehipwell · 2025-12-10T10:34:04Z

github/config.go

 	client.Transport = NewEtagTransport(client.Transport)
 	client.Transport = NewRateLimitTransport(client.Transport, WithWriteDelay(writeDelay), WithReadDelay(readDelay), WithParallelRequests(parallelRequests))
-	client.Transport = logging.NewSubsystemLoggingHTTPTransport("GitHub", client.Transport)
+	client.Transport = logging.NewLoggingHTTPTransport(client.Transport)


Could you explain why we need this change?

stevehipwell · 2025-12-10T10:35:33Z

github/resource_github_organization_ruleset.go

+				ValidateFunc: validation.StringInSlice([]string{"branch", "tag", "push", "repository"}, false),
+				Description:  "The target of the ruleset. Possible values are `branch`, `tag`, `push` and `repository`.",


This needs removing.

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.

nickfloyd reviewed Dec 2, 2025

View reviewed changes

stevehipwell reviewed Dec 3, 2025

View reviewed changes

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

Open

1 task

stevehipwell reviewed Dec 5, 2025

View reviewed changes

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

deiga added 23 commits December 7, 2025 16:48

Ensure tests use GITHUB_TEST_* values for Owner and Organization

9909ecc

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

Update skipUnlessMode to check for enterprise ENV variables

d3bafe6

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

Use testOwnerFunc to set testOwner so that GITHUB_TEST_OWNER wo…

a4f929b

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

Update to use Context aware CRUD functions

6966690

By using the `*Context` CRUD functions we get context pass-through and enable HTTP Req/Resp Debug logging Signed-off-by: Timo Sand <[email protected]>

Ensure update_allows_fetch_and_merge isn't added for Org Ruleset

a60ac7f

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

Update name of resource, to make debugging failing tests easier

3e2cd38

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

Need to have one of repository_name or repository_id defined

d82c614

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

Need to have a valid repo reference

b2c3570

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

Actions repository access level needs to be set properly

6954994

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

Required workflow needs to be in the .github/workflows folder

a41b2f3

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

Update indentation

41b3dec

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

Need to have one of repository_name or repository_id defined

512db50

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

Add allowed_merge_methods to rules for Org & Repo rulesets

405d3b6

This is a required field and the SDK doesn't omit if it's empty Signed-off-by: Timo Sand <[email protected]>

Fixed type conversion for allowed_merge_methods

986ae22

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

Update test content to actually pass the GH API

581ff8c

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

Convert github_repository to use *Context CRUD methods

e97f75c

This enables better debugging during tests as we are able to use get logs for HTTP req/resp Signed-off-by: Timo Sand <[email protected]>

Enable `TestGithubOrganizationRulesets/Creates_and_updates_organizati…

f096f44

…on_rulesets_without_errors` to pass Signed-off-by: Timo Sand <[email protected]>

make fmt

2d77c1e

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

Add workaround for GH API bug that OrgAdmin actor_id is returned as `…

9d22505

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

make fmt

74bcfe7

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

Fix `TestGithubOrganizationRulesets/Creates_organization_ruleset_with…

4808483

…_all_bypass_modes` Main fix: `bypass_actors` is returned as sorted from GH API so tests need re-indexing Signed-off-by: Timo Sand <[email protected]>

deiga added 24 commits December 7, 2025 20:57

Fix tests regarding bypass_actors ordering

f698cd2

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

Rename test resources for easier debugging

3752c16

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

Update descriptions and validations

cf35bf9

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

Add CustomizeDiff logic to validate on plan

35ff668

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

Allow organization in tests as well

a944da8

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

Remove unused leftover

e471a58

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

Add first validation test

92fc538

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

Update formatting

4f2ea6a

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

Add validation error when conditions is missing

0e81c85

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

Add further validation tests

056a70f

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

Remove unnecessary skip blocks as individual and anonymous access…

740c5e2

… 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

4fc2f51

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

Add Debug logging with tflog to validation

a70831e

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

Fix validation as ref_name, repository_name and repository_id a…

9d85734

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

Correctly use enterprise runner

5b88bb2

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

Fix test output expectation

08a73fc

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

Remove unnecessary panic test

3f34fb6

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

Improve validation output messages

776fc64

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

Update to use Context aware CRUD functions

7f65508

By using the `*Context` CRUD functions we get context pass-through and enable HTTP Req/Resp Debug logging Signed-off-by: Timo Sand <[email protected]>

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

2479b73

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

Add validation to required_workflow.path

4666bbe

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

make fmt

e9c10b4

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

Rename test resources for easier debugging

519ea54

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

Fix linter issues

21e2e02

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

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

deiga added 2 commits December 8, 2025 15:40

Add validation to ensure `rules.required_status_checks.required_check…

77e8cfe

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

go mod tidy

6782aab

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

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

Add test to ensure that required_checks is always required

97e2351

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

stevehipwell requested changes Dec 10, 2025

View reviewed changes

		ValidateFunc: validation.StringInSlice([]string{"branch", "tag", "push", "repository"}, false),
		Description: "The target of the ruleset. Possible values are `branch`, `tag`, `push` and `repository`.",


		var ruleset *github.RepositoryRuleset

		ruleset, _, err = client.Organizations.UpdateRepositoryRuleset(ctx, org, rulesetID, *rulesetReq)

[MAINT] Fix github_organization_ruleset with push target #2958

Are you sure you want to change the base?

[MAINT] Fix github_organization_ruleset with push target #2958

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!

Choose a reason for hiding this comment

Uh oh!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

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

deiga commented Dec 2, 2025 •

edited

Loading