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

Refactor: cleanup usage of parseTwoPartID #313

Merged
merged 2 commits into from
Feb 4, 2020
Merged

Refactor: cleanup usage of parseTwoPartID #313

merged 2 commits into from
Feb 4, 2020

Conversation

kpfleming
Copy link
Contributor

This function is used in many resources to parse colon-separated
two-part identifier strings, and it emits an error message when
the string does not match the expected format. The error message
indicates that the format was expected to be "organization:name"
but this is only true for some resources, not all of them. As a
result users may get an error while attempting to import a resource
which gives them incorrect information about the identitier format.

This patch improves the function to accept a pair of label strings,
and it uses them when constructing the error message, so the message
will be tailored to the call site's desired format for the string.

Signed-off-by: Kevin P. Fleming kpfleming@bloomberg.net

Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

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

👍 on the tradeoff between end user experience and developer friction.

github/util.go Outdated Show resolved Hide resolved
This function is used in many resources to parse colon-separated
two-part identifier strings, and it emits an error message when
the string does not match the expected format. The error message
indicates that the format was expected to be "organization:name"
but this is only true for some resources, not all of them. As a
result users may get an error while attempting to import a resource
which gives them incorrect information about the identitier format.

This patch improves the function to accept a pair of label strings,
and it uses them when constructing the error message, so the message
will be tailored to the call site's desired format for the string.

Signed-off-by: Kevin P. Fleming <kpfleming@bloomberg.net>
The docstring should refer to the function's argument names and they
were changed from "a, b" to "left, right".

Signed-off-by: Kevin P. Fleming <kpfleming@bloomberg.net>
@jcudit
Copy link
Contributor

jcudit commented Jan 31, 2020

This one triggered 3 failed tests:

------- Stdout: -------
=== RUN   TestAccGithubRepository_createFromTemplate
=== PAUSE TestAccGithubRepository_createFromTemplate
=== CONT  TestAccGithubRepository_createFromTemplate
--- FAIL: TestAccGithubRepository_createFromTemplate (4.26s)
    testing.go:569: Step 0 error: errors during apply:
        
        Error: Provider produced inconsistent result after apply
        
        When applying changes to github_repository.foo, provider "github" produced an
        unexpected new value for was present, but now absent.
        
        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
        
FAIL
------- Stdout: -------
=== RUN   TestAccGithubRepository_autoInitForceNew
=== PAUSE TestAccGithubRepository_autoInitForceNew
=== CONT  TestAccGithubRepository_autoInitForceNew
--- FAIL: TestAccGithubRepository_autoInitForceNew (17.41s)
    testing.go:630: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: errors during apply: DELETE https://api.github.com/repos/terraformtesting/tf-acc-test-6a4gmvihhb: 404 Not Found []
        
        State: github_repository.foo:
          ID = tf-acc-test-6a4gmvihhb
          provider = provider.github
          allow_merge_commit = true
          allow_rebase_merge = true
          allow_squash_merge = true
          archived = false
          auto_init = true
          default_branch = master
          description = 
          etag = W/"a75ede4cf216525f64e5ae4115e80931"
          full_name = terraformtesting/tf-acc-test-6a4gmvihhb
          git_clone_url = git://github.com/terraformtesting/tf-acc-test-6a4gmvihhb.git
          gitignore_template = Go
          has_downloads = false
          has_issues = false
          has_projects = false
          has_wiki = false
          homepage_url = 
          html_url = https://github.com/terraformtesting/tf-acc-test-6a4gmvihhb
          http_clone_url = https://github.com/terraformtesting/tf-acc-test-6a4gmvihhb.git
          license_template = mpl-2.0
          name = tf-acc-test-6a4gmvihhb
          private = false
          ssh_clone_url = git@github.com:terraformtesting/tf-acc-test-6a4gmvihhb.git
          svn_url = https://github.com/terraformtesting/tf-acc-test-6a4gmvihhb
          template.# = 0
FAIL
------- Stdout: -------
=== RUN   TestAccGithubRepositoryWebhook_basic
=== PAUSE TestAccGithubRepositoryWebhook_basic
=== CONT  TestAccGithubRepositoryWebhook_basic
--- FAIL: TestAccGithubRepositoryWebhook_basic (7.73s)
    testing.go:569: Step 1 error: errors during apply:
        
        Error: PATCH https://api.github.com/repos/terraformtesting/foo-1ywn3a3878/hooks/178788842: 404 Not Found []
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test919770926/main.tf line 16:
          (source code not available)
        
        
FAIL

I've got this queued for investigation but looks more involved than the others.

@jcudit
Copy link
Contributor

jcudit commented Feb 4, 2020

Hmm, I've run the tests locally instead of through CI and am seeing positive results. I feel confident in merging this change to unblock #318.

@jcudit jcudit merged commit 8b3aa4c into integrations:master Feb 4, 2020
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
…rtid

Refactor: cleanup usage of parseTwoPartID
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants