Fix restrict pushes on github_branch_protection. Fix branch protection tests

georgekaz commented 9 months ago

Resolves #594

Before the change?

Currently it is not possible to enable push restrictions to just the default maintainers and administrators of a repository, the current code requires at least one user, group or app to be passed.
Also there is a parameter called blocks_creations which currently does not work unless you also set push_restrictions with some values.
There are 3 properties on the api that control push restriction features, these are restrictsPushes, blocksCreations and pushAllowances. Currently setting a value for pushAllowances (push_restrictions) also enables restrictsPushes, which makes this impossible to fix without creating a breaking change or a change that will result in options fighting for control of the same properties and therefore there always being planned changes (similar to how restrict_dismissals and dismissal_restrictions fight over the same API object called restrictsReviewDismissals).
Lots of the branch protection tests were failing
It was not possible to run only the new branch protection tests because the command TF_LOG=DEBUG TF_ACC=1 go test -v ./... -run ^TestAccGithubBranchProtection matched both the test suite for the graph api and for the rest api (v3) version.
Lots of the constant names did not clearly represent the API field they were mapped to
PROTECTION_REQUIRES_LAST_PUSH_APPROVAL was being set twice in resourceGithubBranchProtectionRead, once in the wrong place and causing a warning [DEBUG] Problem setting 'require_last_push_approval' in X Y branch protection (Z)

The build didn't work since the last merge to main branch

github/resource_github_issue_labels.go:8:2: cannot find module providing package github.com/google/go-github/v52/github: import lookup disabled by -mod=vendor
    (Go version in go.mod is at least 1.14 and vendor directory exists.)
make: *** [GNUmakefile:13: build] Error 1

After the change?

You are now able to set restrict pushes on its own without setting push_restrictions actors (which really maps to pushAllowances on the graph api)
Instead of:
```
blocks_creations = true
push_restrictions = [
"/someuser"
]
```
you now use
```
restrict_pushes {
# (Optional) blocks_creations
blocks_creations = false

# (Optional) push_restrictions
push_restrictions = [
"/someuser"
]
}
```
NOTE: It's tempting to rename push_restrictions to push_allowances to match the API (pushAllowances) and to make more sense, or to push_bypassers or restrict_pushes_bypassers to match similar exiting options such as pull_request_bypassers & force_push_bypassers which map to bypassPullRequestAllowances and bypassForcePushAllowances respectively.

This represents the API and the GUI better:

I fixed all the broken tests on TestAccGithubBranchProtection
I renamed the tests on TestAccGithubBranchProtection to TestAccGithubBranchProtectionV4 so that they can be targeted using TF_LOG=DEBUG TF_ACC=1 go test -v ./... -run ^TestAccGithubBranchProtectionV4 to avoid running the TestAccGithubBranchProtectionV3 tests
I refactored (and a-z sorted) the names of the constants to better match the API fields they represent on the graph api
I fixed the build error caused by a reference to the wrong version of github.com/google/go-github
I fixed the warning caused by the incorrect setting of PROTECTION_REQUIRES_LAST_PUSH_APPROVAL

Pull request checklist

[x] Tests for the changes have been added (for bug fixes / features)
[x] Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

This introduces a breaking change because there was no way to fix the current bug without creating a breaking change.

The impact is that people will need to move their existing push_restrictions lists inside of restrict_pushes{}.

Please see our docs on breaking changes to help!

[x] Yes
[ ] No

georgekaz commented 9 months ago

@kfcampbell Is there any chance anyone might look at my PR?

kfcampbell commented 8 months ago

@georgekaz Sorry for the delays, the holidays have been a slow time. I love the fixes here and I'd love to get them into our next major version, which we're targeting for this month. However, we can't release as-is without state migration, otherwise users are going to be annoyed when their state doesn't work with the new version.

We have some examples in our codebase (search for MigrateState or SchemaVersion for code samples you can use) and there's also docs about this here.

georgekaz commented 8 months ago

@kfcampbell I'll work on that now. If we're upgrading state, do you have any thoughts on changing the name of the existing property from push_restrictions to push_allowances to more closely match the api?

It seems a bit odd to have restrict_pushes and push_restrictions; I thought maybe it would be clearer.

So from:

  push_restrictions = [
    "/georgekaz"
  ]

to:

  restrict_pushes {
    push_allowances = [
      "/georgekaz"
    ]
  }

I'll understand if you prefer not to but I thought I should mention it.

georgekaz commented 8 months ago

The work around the state migration has been pretty confusing. I've figured out that this provider is still using v1 of the terraform-plugin-sdk and I can see that the version in my state file is version 4 which is TF 0.11 compatible despite the fact I'm using TF v1.6.4.

So it seems like I need to use the advice from Terraform v0.11 SDK State Migrations but there's an existing state migration that is using the Terraform v0.12 SDK State Migrations in the branch protection func here. To further confuse matters, the docs say:

State migrations performed with StateUpgraders are compatible with the Terraform 0.11 runtime, if the provider still supports the Terraform 0.11 protocol. Additional MigrateState implementation is not necessary and any existing MigrateState implementations do not need to be converted to StateUpgraders.

Other resources such as resource_github_repository.go are using the MigrateState implementation.

I suppose I should add to the existing migration style that has already been used in resource_github_branch_protection.go

georgekaz commented 8 months ago

@kfcampbell I've added the commit. I've tested it locally and it seems to work but honestly I'm not sure it's doing anything because even after running it, the "schema_version" remains at 1 in the state file. There are no errors though when going from the old schema to the new one and the plan works. As long as things are moved from the old format to the new one, it works.

georgekaz commented 8 months ago

I see you have your own PR https://github.com/integrations/terraform-provider-github/pull/1780 @kfcampbell to upgrade to sdk v2. With that in place, the state migration I've done would be the correct one for sure.

kfcampbell commented 8 months ago

:notebook: I've changed the base of this PR to our v6 branch. #2091 will be the PR to watch for the next major version release and merge to main.

If we're upgrading state, do you have any thoughts on changing the name of the existing property from push_restrictions to push_allowances to more closely match the api?

I like this change! I'd be happy to have this go in at the same time.

The work around the state migration has been pretty confusing. [...] I suppose I should add to the existing migration style that has already been used in resource_github_branch_protection.go

I totally agree; this part of the project is pretty obnoxious to deal with. That sounds like a good idea to me!

kfcampbell commented 8 months ago

I'm going to look at #1780 tomorrow and see if I can get it into a little bit better shape, and that might guide us on the migration issue here.

georgekaz commented 8 months ago

ok that's now changed from push_restrictions to push_allowances. I also did some further testing on the state migration and made a fix and have confirmed it works. The schema_version on the object is now incrementing as expected.

kfcampbell commented 8 months ago

@georgekaz thanks for the work here! I've updated the Hashicorp SDK. Would you mind merging the changes from the v6 branch and validating one more time? Then we can get this in and the major change released, which is exciting!

georgekaz commented 8 months ago

@kfcampbell that's rebased on v6, fixed and tested. However I've found a new problem, unrelated to my changes.

Some of the tests in resource_github_branch_protection_test.go use this block:

            data "github_user" "test" {
              username = "%s"
            }

and when these tests are enabled I get the error:

panic: created_at: '' expected type 'string', got unconvertible type 'github.Timestamp', value: '2012-01-30 16:06:52 +0000 UTC'

goroutine 241 [running]:
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*ResourceData).Set(0xc00071c980, {0x12499a5, 0xa}, {0x122fda0, 0xc000844498})
        /home/george/Dev/terraform-provider-github/vendor/github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema/resource_data.go:233 +0x2b5
github.com/integrations/terraform-provider-github/v6/github.dataSourceGithubUserRead(0x0?, {0x10215c0?, 0xc000054340})
        /home/george/Dev/terraform-provider-github/github/data_source_github_user.go:184 +0xa48

I don't think you need more of that stack trace. It seems that something else has broken the data source for github user.

Running TF_LOG=DEBUG TF_ACC=1 go test -v ./... -run ^TestAccGithubUserDataSource will give you the trace.

I've not dived into this but my guess is that it's the created_at field on the user resource, but would also affect other time fields.

See: https://github.com/integrations/terraform-provider-github/blob/main/github/data_source_github_user.go#L85-L96

and https://github.com/integrations/terraform-provider-github/blob/main/vendor/github.com/google/go-github/v57/github/github-accessors.go#L41-L47

kfcampbell commented 8 months ago

@georgekaz thanks!

Running on the main branch, tests (TestAccGithubBranchProtection) complete for me but they fail:

--- FAIL: TestAccGithubBranchProtection (163.85s)
    --- FAIL: TestAccGithubBranchProtection/configures_default_settings_when_empty (19.46s)
        --- SKIP: TestAccGithubBranchProtection/configures_default_settings_when_empty/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubBranchProtection/configures_default_settings_when_empty/with_an_individual_account (0.00s)
        --- FAIL: TestAccGithubBranchProtection/configures_default_settings_when_empty/with_an_organization_account (19.46s)
    --- FAIL: TestAccGithubBranchProtection/configures_default_settings_when_conversation_resolution_is_true (19.97s)
        --- SKIP: TestAccGithubBranchProtection/configures_default_settings_when_conversation_resolution_is_true/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubBranchProtection/configures_default_settings_when_conversation_resolution_is_true/with_an_individual_account (0.00s)
        --- FAIL: TestAccGithubBranchProtection/configures_default_settings_when_conversation_resolution_is_true/with_an_organization_account (19.97s)
    --- FAIL: TestAccGithubBranchProtection/configures_required_status_checks (16.91s)
        --- SKIP: TestAccGithubBranchProtection/configures_required_status_checks/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubBranchProtection/configures_required_status_checks/with_an_individual_account (0.00s)
        --- FAIL: TestAccGithubBranchProtection/configures_required_status_checks/with_an_organization_account (16.91s)
    --- PASS: TestAccGithubBranchProtection/configures_required_pull_request_reviews (11.37s)
        --- SKIP: TestAccGithubBranchProtection/configures_required_pull_request_reviews/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubBranchProtection/configures_required_pull_request_reviews/with_an_individual_account (0.00s)
        --- PASS: TestAccGithubBranchProtection/configures_required_pull_request_reviews/with_an_organization_account (11.37s)
    --- PASS: TestAccGithubBranchProtection/configures_branch_push_restrictions (14.72s)
        --- SKIP: TestAccGithubBranchProtection/configures_branch_push_restrictions/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubBranchProtection/configures_branch_push_restrictions/with_an_individual_account (0.00s)
        --- PASS: TestAccGithubBranchProtection/configures_branch_push_restrictions/with_an_organization_account (14.72s)
    --- FAIL: TestAccGithubBranchProtection/configures_branch_push_restrictions_with_username (8.11s)
        --- SKIP: TestAccGithubBranchProtection/configures_branch_push_restrictions_with_username/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubBranchProtection/configures_branch_push_restrictions_with_username/with_an_individual_account (0.00s)
        --- FAIL: TestAccGithubBranchProtection/configures_branch_push_restrictions_with_username/with_an_organization_account (8.11s)
    --- PASS: TestAccGithubBranchProtection/configures_force_pushes_and_deletions (16.56s)
        --- SKIP: TestAccGithubBranchProtection/configures_force_pushes_and_deletions/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubBranchProtection/configures_force_pushes_and_deletions/with_an_individual_account (0.00s)
        --- PASS: TestAccGithubBranchProtection/configures_force_pushes_and_deletions/with_an_organization_account (16.56s)
    --- FAIL: TestAccGithubBranchProtection/configures_blocksCreations (14.07s)
        --- SKIP: TestAccGithubBranchProtection/configures_blocksCreations/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubBranchProtection/configures_blocksCreations/with_an_individual_account (0.00s)
        --- FAIL: TestAccGithubBranchProtection/configures_blocksCreations/with_an_organization_account (14.07s)
    --- PASS: TestAccGithubBranchProtection/configures_non-empty_list_of_force_push_bypassers (13.66s)
        --- SKIP: TestAccGithubBranchProtection/configures_non-empty_list_of_force_push_bypassers/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubBranchProtection/configures_non-empty_list_of_force_push_bypassers/with_an_individual_account (0.00s)
        --- PASS: TestAccGithubBranchProtection/configures_non-empty_list_of_force_push_bypassers/with_an_organization_account (13.66s)
    --- PASS: TestAccGithubBranchProtection/configures_empty_list_of_force_push_bypassers (11.04s)
        --- SKIP: TestAccGithubBranchProtection/configures_empty_list_of_force_push_bypassers/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubBranchProtection/configures_empty_list_of_force_push_bypassers/with_an_individual_account (0.00s)
        --- PASS: TestAccGithubBranchProtection/configures_empty_list_of_force_push_bypassers/with_an_organization_account (11.04s)
    --- FAIL: TestAccGithubBranchProtection/configures_non-empty_list_of_pull_request_bypassers (6.84s)
        --- SKIP: TestAccGithubBranchProtection/configures_non-empty_list_of_pull_request_bypassers/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubBranchProtection/configures_non-empty_list_of_pull_request_bypassers/with_an_individual_account (0.00s)
        --- FAIL: TestAccGithubBranchProtection/configures_non-empty_list_of_pull_request_bypassers/with_an_organization_account (6.83s)
    --- PASS: TestAccGithubBranchProtection/configures_empty_list_of_pull_request_bypassers (11.15s)
        --- SKIP: TestAccGithubBranchProtection/configures_empty_list_of_pull_request_bypassers/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubBranchProtection/configures_empty_list_of_pull_request_bypassers/with_an_individual_account (0.00s)
        --- PASS: TestAccGithubBranchProtection/configures_empty_list_of_pull_request_bypassers/with_an_organization_account (11.15s)

Running those tests on the v6 branch, I get a panic: "Invalid address to set: []string{\"require_last_push_approval\"}" on this line.

Running the same tests (TestAccGithubBranchProtectionV4) on this branch, I get what appears to be the panic you're seeing above: panic: created_at: '' expected type 'string', got unconvertible type 'github.Timestamp', value: '2014-10-20 22:56:15 +0000 UTC', which springs from this line.

This is a somewhat obnoxious situation, as the default is broken but arguably less broken than our two more updated scenarios (this branch and v6). Do you have thoughts on what we can do to fix these panics? Perhaps we should take your changes as-is and do a follow-up PR to fix the tests?

georgekaz commented 7 months ago

hmm. Well most of the branch protection tests were failing when I started out this work and I made all the v4 ones pass on my branch. I tried not to get involved with any of the others. I think the panic is most likely related to the sdk upgrade because it's going to be the biggest change, or the recent upgrade to github go lib v57. Have you run those same tests just on the sdk upgrade branch?

I would target the tests on that data rather than the branch protection ones which are failing as a consequence of that.

TF_LOG=DEBUG TF_ACC=1 go test -v ./... -run ^TestAccGithubUserDataSource

It requires GITHUB_OWNER and/or GITHUB_ORGANISATION to be set in order to see the error

I'll take a look and see what I can do

georgekaz commented 7 months ago

@kfcampbell I managed to fix the branch protection and user tests, please see my last commit. I also checked for other timestamp fields that weren't being cast and I found the ones on release. Even after fixing the code, the user tests were failing because it assumed that a name would be returned. However this field is optional an I've not set it on my account, so the test was failing. I changed it to check for login which is a required field of course.

kfcampbell commented 7 months ago

@georgekaz you rock! This fixes nearly all the branch protection tests; I see that configures_branch_push_restrictions_with_username and empty_list_of_pull_request_bypassers are failing for me, but this is a much healthier state than the main branch, so thanks a bunch.

Are you ready for this to go into the v6 branch? I can cut another beta version for that release afterward.

georgekaz commented 7 months ago

thanks @kfcampbell .

Both those tests pass for me, do you have the env vars set?

GITHUB_OWNER=georgekaz
GITHUB_ORGANIZATION=georgekaz-org

--- PASS: TestAccGithubBranchProtectionV4 (43.42s)
    --- PASS: TestAccGithubBranchProtectionV4/configures_branch_push_restrictions_with_username (15.47s)
        --- SKIP: TestAccGithubBranchProtectionV4/configures_branch_push_restrictions_with_username/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubBranchProtectionV4/configures_branch_push_restrictions_with_username/with_an_individual_account (0.00s)
        --- PASS: TestAccGithubBranchProtectionV4/configures_branch_push_restrictions_with_username/with_an_organization_account (15.47s)
    --- PASS: TestAccGithubBranchProtectionV4/configures_empty_list_of_pull_request_bypassers (27.95s)
        --- SKIP: TestAccGithubBranchProtectionV4/configures_empty_list_of_pull_request_bypassers/with_an_anonymous_account (0.00s)
        --- PASS: TestAccGithubBranchProtectionV4/configures_empty_list_of_pull_request_bypassers/with_an_individual_account (14.09s)
        --- PASS: TestAccGithubBranchProtectionV4/configures_empty_list_of_pull_request_bypassers/with_an_organization_account (13.86s)
=== RUN   TestAccGithubBranchProtectionV4StateUpgradeV1
--- PASS: TestAccGithubBranchProtectionV4StateUpgradeV1 (0.00s)
PASS
ok      github.com/integrations/terraform-provider-github/v6/github     43.433s

I think this problem is fixed and it's ready to merge, although feel free to cut another beta first if you prefer.

I do think a separate task to review all the tests is probably in required. As I say, when I started working on this hardly any of the existing branch protection tests were passing and I had to fix all those. It now seems that the user data tests weren't reliable either.

kfcampbell commented 7 months ago

Also, I really really appreciate all the love and attention paid to fixing our tests here. Thanks a bunch!

integrations / terraform-provider-github