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

Bug Fix: Webhook secret stored incorrectly in state #251

Merged
merged 2 commits into from
Jul 15, 2019

Conversation

megan07
Copy link
Contributor

@megan07 megan07 commented Jul 2, 2019

Addressing #171 where the webhook secret set in Terraform makes signatures not match.
Before fix:

--- FAIL: TestAccGithubRepositoryWebhook_importSecret (9.38s)
    testing.go:568: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

        (map[string]string) (len=1) {
         (string) (len=22) "configuration.0.secret": (string) (len=8) "********"
        }


        (map[string]string) (len=1) {
         (string) (len=22) "configuration.0.secret": (string) (len=18) "RandomSecretString"
        }

After fix:

=== RUN   TestAccGithubRepositoryWebhook_importSecret
=== PAUSE TestAccGithubRepositoryWebhook_importSecret
=== CONT  TestAccGithubRepositoryWebhook_importSecret
--- PASS: TestAccGithubRepositoryWebhook_importSecret (9.66s)
PASS

@ghost ghost added size/S Type: Documentation Improvements or additions to documentation labels Jul 2, 2019
@megan07 megan07 requested a review from a team July 2, 2019 14:38
Copy link
Contributor

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

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

I'd love to see a test that the secret actually ends up in state as we expect. Otherwise, this looks reasonable to me.

@@ -92,6 +92,11 @@ func resourceGithubOrganizationWebhookCreate(d *schema.ResourceData, meta interf
}
d.SetId(strconv.FormatInt(*hook.ID, 10))

if hook.Config["secret"] != nil {
hook.Config["secret"] = webhookObj.Config["secret"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming the GitHub API doesn't return the secret, so we have to set it based on what's in the ResourceData? If so, can we add a comment saying as much, just to explain why we have to do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, what does hook.Config get set to, if not nil? That looks like the GitHub response, but what does it actually get set to?

currentSecret := d.Get("configuration").([]interface{})[0].(map[string]interface{})["secret"]

if hook.Config["secret"] != nil {
hook.Config["secret"] = currentSecret
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise, a comment here just explaining the situation would be 💯

@harlowja
Copy link

harlowja commented Jul 9, 2019

Ah, this explains why terraform screwed up all of our organizations hook secrets! Can we get this merged....

@ghost ghost added size/M and removed size/S labels Jul 9, 2019
@zhenkaiwang
Copy link

Hope this get reviewed and merged soon 🚀

@wenbin1989
Copy link

Hope this get merged ASAP. we have to apply the patch manually currently, which is not very nice.

@megan07 megan07 requested a review from paddycarver July 10, 2019 17:17
@paddycarver paddycarver merged commit 9df5331 into master Jul 15, 2019
@paultyng paultyng deleted the b-webhook-secret branch September 8, 2020 13:23
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
…k-secret

Bug Fix: Webhook secret stored incorrectly in state
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Type: Documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants