integrations / terraform-provider-github

Terraform GitHub provider
https://www.terraform.io/docs/providers/github/
MIT License
879 stars 719 forks source link

[BUG]: Since `6.0.1` the `github_actions_repository_permissions` resource errors on read when it's disabled #2182

Closed stevehipwell closed 5 months ago

stevehipwell commented 5 months ago

Expected Behavior

When a github_actions_repository_permissions resource is set to disabled it should be able to be read correctly.

Actual Behavior

We're getting the following error when we have a github_actions_repository_permissions resource set to disabled.

╷ │ Error: GET https://api.github.com/repos/my-org/test/actions/permissions/selected-actions: 409 [] │ │ with module.repo["test"].github_actions_repository_permissions.default, │ on modules/repo/main.tf line 72, in resource "github_actions_repository_permissions" "default": │ 72: resource "github_actions_repository_permissions" "default" { │ ╵

I suspect that this has been caused by the changes in https://github.com/integrations/terraform-provider-github/commit/97d51139cde3bcb2d7838844e8f24086e4830e52.

Terraform Version

Terraform v1.5.7 on linux_amd64

Affected Resource(s)

Terraform Configuration Files

locals {
  actions_enabled = false
}

resource "github_actions_repository_permissions" "default" {
  repository      = "my-repo"
  enabled         = local.actions_enabled
  allowed_actions = local.actions_enabled ? "all" : null
}

Steps to Reproduce

No response

Debug Output

No response

Panic Output

No response

Code of Conduct

kfcampbell commented 5 months ago

Oh, that's not ideal. Do you think this is severe enough to warrant a rollback of #2114? @Danielku15, do you have thoughts on how we might go about resolving this?

stevehipwell commented 5 months ago

@kfcampbell from our side it wasted a couple of hours and then required a rollback to v6.0.0. I assume it could be fixed forward but if not it'd probably warrant a rollback. I think it'd be worth getting a regression test in either way.

Danielku15 commented 5 months ago

I'll try to have a look tomorrow (its already 11pm here) 😅 From my direct changes I cannot directly infer what would trigger this. My new conditions mainly target the allowed_actions="selected". I assume they trigger some unintended else-path. Should be easy to fix once I narrowed down the exact path.

Edit: The TestAccGithubActionsRepositoryPermissions/test_disabling_actions_on_a_repository suite also fails already on my PC with this error. Seems tests are not fully running on GitHub Actions / PullRequests?

@kfcampbell Would it make sense to setup there a more continuous acceptance testing to catch those errors? https://github.com/integrations/terraform-provider-github/actions/runs/8144672944/job/22259269917#step:8:7

Found the error and will work on a fix. The problematic code is the resource import / initial read logic where terraform does not provide access to the desired state defined in code and we try to load the actions always:

https://github.com/integrations/terraform-provider-github/blob/b9af9f3b0c8fe2c7d3c7a0874426ad10ecf5e14a/github/resource_github_actions_repository_permissions.go#L177-L179

Edit 2: Fix is ready at https://github.com/integrations/terraform-provider-github/pull/2186

kfcampbell commented 5 months ago

@Danielku15 thanks for the attention here! You're correct that integration tests are not automatically running on PRs and merges; I've opened https://github.com/integrations/terraform-provider-github/issues/1414 to track this but we haven't gotten the time or priority to work on it unfortunately. We've struggled with the expense, time, and hassle of automatically testing resources and we're certainly in an unhealthy position now. In the meantime, I've been running relevant tests manually, which is not ideal.

I appreciate you opening the PR, I'll look shortly!

stevehipwell commented 5 months ago

@kfcampbell it looks like the v6.1.0 release hasn't made it to the registry yet? Is this expected?

kfcampbell commented 5 months ago

It's definitely not. Let's keep future discussion about that issue on #2191.

stevehipwell commented 5 months ago

2191 wasn't created when I added this comment, but I saw it was opened and am following it.