Unify log messages & 404 detection approach

[radeksimko]

TF_ACC=1 go test ./github -v  -timeout 120m
=== RUN   TestAccGithubIpRangesDataSource_existing
--- PASS: TestAccGithubIpRangesDataSource_existing (1.15s)
=== RUN   TestAccGithubRepositoriesDataSource_basic
--- PASS: TestAccGithubRepositoriesDataSource_basic (7.43s)
=== RUN   TestAccGithubRepositoriesDataSource_noMatch
--- PASS: TestAccGithubRepositoriesDataSource_noMatch (0.90s)
=== RUN   TestAccGithubRepositoryDataSource_fullName_noMatchReturnsError
--- PASS: TestAccGithubRepositoryDataSource_fullName_noMatchReturnsError (0.24s)
=== RUN   TestAccGithubRepositoryDataSource_name_noMatchReturnsError
--- PASS: TestAccGithubRepositoryDataSource_name_noMatchReturnsError (0.15s)
=== RUN   TestAccGithubRepositoryDataSource_fullName_existing
--- PASS: TestAccGithubRepositoryDataSource_fullName_existing (1.62s)
=== RUN   TestAccGithubRepositoryDataSource_name_existing
--- PASS: TestAccGithubRepositoryDataSource_name_existing (3.23s)
=== RUN   TestAccGithubTeamDataSource_noMatchReturnsError
--- PASS: TestAccGithubTeamDataSource_noMatchReturnsError (0.22s)
=== RUN   TestAccGithubUserDataSource_noMatchReturnsError
--- PASS: TestAccGithubUserDataSource_noMatchReturnsError (0.15s)
=== RUN   TestAccGithubUserDataSource_existing
--- PASS: TestAccGithubUserDataSource_existing (5.04s)
=== RUN   TestMigrateGithubWebhookStateV0toV1
--- PASS: TestMigrateGithubWebhookStateV0toV1 (0.00s)
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestProvider_insecure
--- PASS: TestProvider_insecure (0.14s)
=== RUN   TestAccGithubBranchProtection_basic
--- FAIL: TestAccGithubBranchProtection_basic (4.67s)
	testing.go:518: Step 0 error: Check failed: Check 3/15 error: diff "restrictions.users": (-got +want)
		 [
		+ "terraformcollaborator",
		 ]
=== RUN   TestAccGithubBranchProtection_teams
--- PASS: TestAccGithubBranchProtection_teams (13.24s)
=== RUN   TestAccGithubBranchProtection_emptyItems
--- PASS: TestAccGithubBranchProtection_emptyItems (4.67s)
=== RUN   TestAccGithubBranchProtection_importBasic
--- FAIL: TestAccGithubBranchProtection_importBasic (4.81s)
	testing.go:518: Step 0 error: After applying this step, the plan was not empty:

		DIFF:

		UPDATE: github_branch_protection.master
		  required_pull_request_reviews.0.dismissal_users.#:         "0" => "1"
		  required_pull_request_reviews.0.dismissal_users.862080235: "" => "terraformcollaborator"
		  restrictions.0.users.#:                                    "0" => "1"
		  restrictions.0.users.862080235:                            "" => "terraformcollaborator"
...
=== RUN   TestAccGithubIssueLabel_basic
--- PASS: TestAccGithubIssueLabel_basic (6.15s)
=== RUN   TestAccGithubIssueLabel_existingLabel
--- PASS: TestAccGithubIssueLabel_existingLabel (3.73s)
=== RUN   TestAccGithubIssueLabel_importBasic
--- PASS: TestAccGithubIssueLabel_importBasic (3.85s)
=== RUN   TestAccGithubIssueLabel_description
--- PASS: TestAccGithubIssueLabel_description (10.94s)
=== RUN   TestAccGithubMembership_basic
--- PASS: TestAccGithubMembership_basic (2.00s)
=== RUN   TestAccGithubMembership_importBasic
--- PASS: TestAccGithubMembership_importBasic (1.28s)
=== RUN   TestAccGithubOrganizationProject_basic
--- PASS: TestAccGithubOrganizationProject_basic (1.54s)
=== RUN   TestAccGithubOrganizationProject_importBasic
--- PASS: TestAccGithubOrganizationProject_importBasic (2.40s)
=== RUN   TestAccGithubOrganizationWebhook_basic
--- PASS: TestAccGithubOrganizationWebhook_basic (4.02s)
=== RUN   TestAccGithubOrganizationWebhook_secret
--- PASS: TestAccGithubOrganizationWebhook_secret (1.49s)
=== RUN   TestAccGithubRepositoryCollaborator_basic
--- PASS: TestAccGithubRepositoryCollaborator_basic (6.20s)
=== RUN   TestAccGithubRepositoryCollaborator_importBasic
--- PASS: TestAccGithubRepositoryCollaborator_importBasic (4.56s)
=== RUN   TestSuppressDeployKeyDiff
--- PASS: TestSuppressDeployKeyDiff (0.00s)
=== RUN   TestAccGithubRepositoryDeployKey_basic
--- PASS: TestAccGithubRepositoryDeployKey_basic (5.35s)
=== RUN   TestAccGithubRepositoryDeployKey_importBasic
--- PASS: TestAccGithubRepositoryDeployKey_importBasic (3.70s)
=== RUN   TestAccGithubRepositoryProject_basic
--- PASS: TestAccGithubRepositoryProject_basic (3.76s)
=== RUN   TestAccGithubRepositoryProject_importBasic
--- PASS: TestAccGithubRepositoryProject_importBasic (3.90s)
=== RUN   TestAccGithubRepository_basic
--- PASS: TestAccGithubRepository_basic (4.39s)
=== RUN   TestAccGithubRepository_archive
--- PASS: TestAccGithubRepository_archive (2.56s)
=== RUN   TestAccGithubRepository_archiveUpdate
--- PASS: TestAccGithubRepository_archiveUpdate (4.10s)
=== RUN   TestAccGithubRepository_importBasic
--- PASS: TestAccGithubRepository_importBasic (2.52s)
=== RUN   TestAccGithubRepository_defaultBranch
--- PASS: TestAccGithubRepository_defaultBranch (6.47s)
=== RUN   TestAccGithubRepository_templates
--- PASS: TestAccGithubRepository_templates (3.31s)
=== RUN   TestAccGithubRepository_topics
--- PASS: TestAccGithubRepository_topics (6.41s)
=== RUN   TestAccGithubRepository_autoInitForceNew
--- PASS: TestAccGithubRepository_autoInitForceNew (7.39s)
=== RUN   TestAccGithubRepositoryWebhook_basic
--- PASS: TestAccGithubRepositoryWebhook_basic (10.79s)
=== RUN   TestAccGithubRepositoryWebhook_secret
--- PASS: TestAccGithubRepositoryWebhook_secret (5.44s)
=== RUN   TestAccGithubRepositoryWebhook_importBasic
--- PASS: TestAccGithubRepositoryWebhook_importBasic (3.53s)
=== RUN   TestAccGithubRepositoryWebhook_importSecret
--- PASS: TestAccGithubRepositoryWebhook_importSecret (3.32s)
=== RUN   TestAccGithubTeamMembership_basic
--- PASS: TestAccGithubTeamMembership_basic (4.77s)
=== RUN   TestAccGithubTeamMembership_importBasic
--- PASS: TestAccGithubTeamMembership_importBasic (4.61s)
=== RUN   TestAccGithubTeamRepository_basic
--- PASS: TestAccGithubTeamRepository_basic (9.58s)
=== RUN   TestAccGithubTeamRepository_importBasic
--- PASS: TestAccGithubTeamRepository_importBasic (5.70s)
=== RUN   TestAccCheckGetPermissions
--- PASS: TestAccCheckGetPermissions (0.00s)
=== RUN   TestAccGithubTeam_basic
--- PASS: TestAccGithubTeam_basic (3.11s)
=== RUN   TestAccGithubTeam_slug
--- PASS: TestAccGithubTeam_slug (1.92s)
=== RUN   TestAccGithubTeam_hierarchical
--- PASS: TestAccGithubTeam_hierarchical (3.62s)
=== RUN   TestAccGithubTeam_importBasic
--- PASS: TestAccGithubTeam_importBasic (1.55s)
=== RUN   TestAccGithubUserGpgKey_basic
--- PASS: TestAccGithubUserGpgKey_basic (1.16s)
=== RUN   TestAccGithubUserSshKey_basic
--- PASS: TestAccGithubUserSshKey_basic (1.68s)
=== RUN   TestAccGithubUserSshKey_importBasic
--- PASS: TestAccGithubUserSshKey_importBasic (1.23s)
=== RUN   TestAccGithubUtilRole_validation
--- PASS: TestAccGithubUtilRole_validation (0.00s)
=== RUN   TestAccGithubUtilTwoPartID
--- PASS: TestAccGithubUtilTwoPartID (0.00s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-github/github	211.732s

The two failures are a result of eventually consistent API, at least I can see these failing on master too currently.

[appilon]

Overall LGTM, just made some minor comments/suggestions/inquiries.

[appilon]

nitpick: prefer http.StatusNotFound over hardcoding http response codes.

[appilon]

I actually meant the whole block, error cast, 404 check, and then print message. Like i said though its no biggie

…

On Fri, Aug 31, 2018, 11:55 AM Radek Simko, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In github/resource_github_user_gpg_key.go <https://github.com/terraform-providers/terraform-provider-github/pull/142#discussion_r214399499> : > key, _, err := client.Users.GetGPGKey(context.TODO(), id) if err != nil { - log.Printf("[WARN] GitHub User GPG Key (%s) not found, removing from state", d.Id()) - d.SetId("") - return nil + if err, ok := err.(*github.ErrorResponse); ok { I'm torn on that... I see your point, but at the same time it wouldn't save a single LOC, just a few characters 🤔 We still need both variables, so effectively you'd end up with something like if err, ok := tryCastingToGithubError(err); ok { the logic we'd decouple would be just attempt to cast the error, so I see very little benefit in decoupling it. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/terraform-providers/terraform-provider-github/pull/142#discussion_r214399499>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AjIEHVuSyOg1fu0RJgzZk_QjOzH9d5L5ks5uWVx_gaJpZM4WVbY-> .

[radeksimko]

I actually meant the whole block, error cast, 404 check, and then print message.

Right, that's intentionally verbose for now, because I'm lining up another PR which will add another check for http.StatusNotModified, so such abstraction would probably get in the way. 🙂