feat: allow emtpy invitation IDs

[morremeyer]

Part of #1157.

---

Behavior

Before the change?

github_user_invitation_accepter required an invitation_id to be set.

This made it impossible to use a github_user_invitation_accepter with for_each since
a github_repository_collaborator.invitation_id will be empty after a state refresh when
the invitation has been accepted.

After the change?

The new argument allow_empty_id can be set to true, allowing the invitation_id to be empty.
In that case, the resource does nothing.

If the invitation_id is set, the behavior is the same as before.

This allows for automated adding of a collaborator to a changing list of repositories (e.g. with
the github_repositories datasource) without the plan to fail after a state refresh.

Other information

Note that as reported in #1157, when an invitation is accepted between the state refresh and the apply,
this will still result in a 404 error for the invitation in the API.

---

Additional info

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)
• Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

• Yes (Please add the Type: Breaking change label)
• No

Pull request type

Type: Feature

[kfcampbell]

When attempting to run the new integration test you've added, I get a panic: "interface conversion: interface {} is nil, not bool" on line 37 of github/resource_github_user_invitation_accepter.go here.

Can you reproduce this or does it succeed for you?

[morremeyer]

@kfcampbell I was under the assumption that the tests ran automatically on PR. Running it locally gave me the same result and I fixed the bug you encountered.

I had to however comment out the PreCheck for the test (but I think it is needed, correct?) since the required environment variables for acceptance tests in testAccPreCheck are all undocumented and I have no idea what to set for them.

If you could enlighten me what I need to do for the tests to work there, I'd be happy about it 😊

[kfcampbell]

I was under the assumption that the tests ran automatically on PR

Unfortunately only our unit tests run on PR, and the vast majority of our tests are integration tests. I run the relevant tests manually before merging any PRs, but it would be great for us to do this in an automated fashion. There's a previous attempt here that's been broken for quite some time now. In #1414 we've outlined work to start approaching this problem, though we've been unable to prioritize it unfortunately. It's probably the single biggest issue I have with the provider right now. Apologies!

I had to however comment out the PreCheck for the test (but I think it is needed, correct?)

You're correct; it is needed.

the required environment variables for acceptance tests in testAccPreCheck are all undocumented and I have no idea what to set for them

If you could enlighten me what I need to do for the tests to work there, I'd be happy about it

I'm sorry this isn't ideal! It bothers me as well (see again #1414). Here are some thoughts about what I do:

• GITHUB_TEST_USER: usually my own account (@kfcampbell)
• GITHUB_TEST_COLLABORATOR: an account you control where you'll be able to view invites (for this I created @kfcampbell-terraform-test-user)
• GITHUB_TEMPLATE_REPOSITORY: I use a repo generated from this repo as a test repo (in my test org @kfcampbell-terraform-provider, it's a private template)
• GITHUB_TEMPLATE_REPOSITORY_RELEASE_ID: first, create a release in your template repo; I do this manually in the UI. After that, you can use the Releases API or better yet, the Go SDK, to list releases and grab the ID to export here.

I tend to prefer to run integration tests in VSCode using a launch.json file of the kind below:

{
	// for information on how to debug the provider, see the CONTRIBUTING.md file
	"version": "0.2.0",
	"configurations": [
		{
			"name": "Launch test function",
			"type": "go",
			"request": "launch",
			"mode": "test",
			"program": "/home/kfcampbell/github/dev/terraform-provider-github/github/resource_github_team_members_test.go", // any Go file in the github package works here
			"args": [
				"-test.v",
				"-test.run",
				"^TestAccGithubUserInvitationAccepterAllowEmptyId$"
			],
			"env": {
				"GITHUB_TEST_COLLABORATOR": "kfcampbell-terraform-test-user",
				"GITHUB_TEST_COLLABORATOR_TOKEN": "REDACTED",
				"GITHUB_TEST_USER": "kfcampbell",
				"GITHUB_TOKEN": "REDACTED",
				"GITHUB_TEMPLATE_REPOSITORY": "terraform-template-module",
				"GITHUB_TEMPLATE_REPOSITORY_RELEASE_ID": "REDACTED",
				//"GITHUB_OWNER": "kfcampbell-terraform-provider",
				"GITHUB_ORGANIZATION": "kfcampbell-terraform-provider", // GITHUB_ORGANIZATION is required for organization integration tests
				"TF_ACC": "1",
				"TF_LOG": "DEBUG",
				"APP_INSTALLATION_ID": "REDACTED"
			}
		}
	]
}

If you read that and think, "wow, that's way too much work", you'd be absolutely correct.

For what it's worth, when running the tests above, I'm no longer seeing a panic but I do see an error:

        Error: Provider produced inconsistent result after apply
        
        When applying changes to github_user_invitation_accepter.test, provider
        "github" produced an unexpected new value for was present, but now absent.

In the above error message, I would normally expect to see the second sentence read something like "...produced an unexpected value for {someField}: was {previousValue}, but now absent." This makes me hypothesize that it might be related to the nil return value here without setting any form of ID like we do here. I'm not positive though!

[morremeyer]

@kfcampbell Thanks for the detailed explanation. I now have a working setup for the tests 🎉

I pushed e99eca0 to fix the failing test.

This makes me hypothesize that it might be related to the nil return value here without setting any form of ID like we do here. I'm not positive though!

You were correct about this. The problem here is that we only determine on resource creation that we actually don't want to do anything. But there is no way to use d.SetId("") in the Create operation since this will destroy the resource immediately - that's what I think also led to the weird error message.

We want to create the resource, we just don't want the provider to do anything with it. We also don't have any kind of unique identifier to use as ID for the resource.
I therefore decided to set the resource ID to a UUID since we can be practically certain that these are unique.

I'd be very happy about a review here now 😊

[kfcampbell]

@morremeyer have you validated this behavior manually? I'm reading this doc about Terraform provider development and it includes this snippet:

The existence of a non-blank ID tells Terraform that a resource was created. This ID can be any string value, but should be a value that Terraform can use to read the resource again. Since this data resource doesn't have a unique ID, you set the ID to the current UNIX time, which will force this resource to refresh during every Terraform apply.

This made me think using a random UUID could have unintended consequences, though it sounds like a reasonable thing to do and your tests look good. If you'd like this merged and released, I can do so and we can iterate in the future if necessary!

[morremeyer]

I validated it by running the test with the code setting d.SetId(''), which led to the same error you had before.

I think we'll be fine with a UUID since the resources are never read later - the read operation is noop, see

terraform-provider-github/github/resource_github_user_invitation_accepter.go

Line 15 in fb73a4a

Read: schema.Noop, // Nothing to read as invitation is removed after it's accepted

.

As far as I understand it, with the current code it will save the resource to the state with the UUID an ID.

When it comes to reading it, nothing happens. When it comes to deleting it, it's just removed from the state. Which is why I think the UUIDs are fine.

If it turns out they are not, yes, let's iterate.

I'd be very happy about this being released as it will unblock us 😊

[kfcampbell]

Let's do it! I'll release this shortly.

[morremeyer]

We updated to v5.17.0 earlier and it works like a charm so far. Thanks for your review and all the input before!