Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add error message for importing branch protection #721

Merged

Conversation

tibbes
Copy link
Contributor

@tibbes tibbes commented Mar 8, 2021

@bateller reported (as a follow-up on #671) that the import function for github_branch_protection doesn't give a nice error message if the resource doesn't exist on GitHub.

Error: nil entry in ImportState results. This is always a bug with
the resource that is being imported. Please report this as
a bug to Terraform.

This pull request changes the message to:

Error: Could not find a branch protection rule with the pattern 'master'.

The trouble is, I don't know how to add an idiomatic test for this change. Any advice would be much appreciated.

Add a useful error message when a user attempts to import a branch
protection rule that does not exist.

Before:

Error: nil entry in ImportState results. This is always a bug with
the resource that is being imported. Please report this as
a bug to Terraform.

After:

Error: Could not find a branch protection rule with the pattern 'master'.
Copy link

@bateller bateller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Also this closes #597
Disregard, we'll need another PR to fix importing by branch_id

@jcudit
Copy link
Contributor

jcudit commented Mar 16, 2021

This looks like it'll fix our issue and tests are passing locally for existing functionality. @tibbes happy to merge as-is but will also leave some guidance on a possible approach to testing if you'd like to follow up there:

  • Take a look at the TestStep struct, specifically its ExpectError feature
  • When defining test cases, there is the ImportState field that can be used to make a test step perform a terraform import of a resource created from a previous step. Our webhook test does this:
		testCase := func(t *testing.T, mode string) {
			resource.Test(t, resource.TestCase{
				PreCheck:  func() { skipUnlessMode(t, mode) },
				Providers: testAccProviders,
				Steps: []resource.TestStep{
					{
						Config: config,
						Check:  check,
					},
					{
						ResourceName:      "github_organization_webhook.test",
						ImportState:       true,
						ImportStateVerify: true,
					},
				},
			})
		}

@tibbes
Copy link
Contributor Author

tibbes commented Mar 16, 2021

@jcudit, thanks for pointing out ExpectError. That's just what I needed. I've added tests for the error message now.

@tibbes tibbes closed this Mar 17, 2021
@tibbes tibbes deleted the import-branch-protection-error-message branch March 17, 2021 21:44
@tibbes tibbes restored the import-branch-protection-error-message branch March 17, 2021 22:09
@tibbes tibbes reopened this Mar 17, 2021
@tibbes
Copy link
Contributor Author

tibbes commented Mar 17, 2021

(sorry—accidental close of PR)

@jcudit jcudit added this to the v4.6.0 milestone Mar 22, 2021
@jcudit jcudit merged commit 237d951 into integrations:master Mar 24, 2021
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
* Add error message for importing branch protection

Add a useful error message when a user attempts to import a branch
protection rule that does not exist.

Before:

Error: nil entry in ImportState results. This is always a bug with
the resource that is being imported. Please report this as
a bug to Terraform.

After:

Error: Could not find a branch protection rule with the pattern 'master'.

* Test error message for importing branch protection

* Simplify code in branch protection import
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants