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

resource/github_repository_webhook: Avoid spurious diff #133

Merged
merged 1 commit into from
Aug 14, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions github/migrate_github_repository_webhook.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package github

import (
"fmt"
"log"
"strings"

"github.com/hashicorp/terraform/terraform"
)

func resourceGithubRepositoryWebhookMigrateState(v int, is *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) {
switch v {
case 0:
log.Println("[INFO] Found GitHub Repository Webhook State v0; migrating to v1")
return migrateGithubRepositoryWebhookStateV0toV1(is)
default:
return is, fmt.Errorf("Unexpected schema version: %d", v)
}
}

func migrateGithubRepositoryWebhookStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceState, error) {
if is.Empty() {
log.Println("[DEBUG] Empty InstanceState; nothing to migrate.")
return is, nil
}

log.Printf("[DEBUG] GitHub Repository Webhook Attributes before migration: %#v", is.Attributes)

prefix := "configuration."

delete(is.Attributes, prefix+"%")

// Read & delete old keys
oldKeys := make(map[string]string, 0)
for k, v := range is.Attributes {
if strings.HasPrefix(k, prefix) {
oldKeys[k] = v

// Delete old keys
delete(is.Attributes, k)
}
}

// Write new keys
for k, v := range oldKeys {
Copy link
Contributor

Choose a reason for hiding this comment

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

any chance this could be done all in the previous loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was actually the very first version of this PR 😃until I saw the test intermittently failing as the for loop kept ranging over some new elements which were just added.

Why? https://golang.org/ref/spec#For_range

The iteration order over maps is not specified and is not guaranteed to be the same from one iteration to the next. If a map entry that has not yet been reached is removed during iteration, the corresponding iteration value will not be produced. If a map entry is created during iteration, that entry may be produced during the iteration or may be skipped. The choice may vary for each entry created and from one iteration to the next.

Short answer: No.

Feel free to have a play yourself: https://play.golang.org/p/Tm1FPyjwd0m 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhhh yes I see now. 👍

newKey := "configuration.0." + strings.TrimPrefix(k, prefix)
is.Attributes[newKey] = v
}

is.Attributes[prefix+"#"] = "1"

log.Printf("[DEBUG] GitHub Repository Webhook Attributes after State Migration: %#v", is.Attributes)

return is, nil
}
38 changes: 38 additions & 0 deletions github/migrate_github_repository_webhook_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package github

import (
"reflect"
"testing"

"github.com/hashicorp/terraform/terraform"
)

func TestMigrateGithubRepositoryWebhookStateV0toV1(t *testing.T) {
oldAttributes := map[string]string{
"configuration.%": "4",
"configuration.content_type": "form",
"configuration.insecure_ssl": "0",
"configuration.secret": "blablah",
"configuration.url": "https://google.co.uk/",
}

newState, err := migrateGithubRepositoryWebhookStateV0toV1(&terraform.InstanceState{
ID: "nonempty",
Attributes: oldAttributes,
})
if err != nil {
t.Fatal(err)
}

expectedAttributes := map[string]string{
"configuration.#": "1",
"configuration.0.content_type": "form",
"configuration.0.insecure_ssl": "0",
"configuration.0.secret": "blablah",
"configuration.0.url": "https://google.co.uk/",
}
if !reflect.DeepEqual(newState.Attributes, expectedAttributes) {
t.Fatalf("Expected attributes:\n%#v\n\nGiven:\n%#v\n",
expectedAttributes, newState.Attributes)
}
}
43 changes: 40 additions & 3 deletions github/resource_github_repository_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ func resourceGithubRepositoryWebhook() *schema.Resource {
},
},

SchemaVersion: 1,
MigrateState: resourceGithubRepositoryWebhookMigrateState,

Schema: map[string]*schema.Schema{
"name": {
Type: schema.TypeString,
Expand All @@ -46,8 +49,38 @@ func resourceGithubRepositoryWebhook() *schema.Resource {
Set: schema.HashString,
},
"configuration": {
Type: schema.TypeMap,
Type: schema.TypeList,
MaxItems: 1,
Optional: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"url": {
Type: schema.TypeString,
Required: true,
},
"content_type": {
Type: schema.TypeString,
Optional: true,
},
"secret": {
Type: schema.TypeString,
Optional: true,
Sensitive: true,
DiffSuppressFunc: func(k, oldV, newV string, d *schema.ResourceData) bool {
maskedSecret := "********"
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't really worked with sensitive attributes, but it feels strange to me for a maskedSecret to ALWAYS be 8 asterisks, is this code robust?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GitHub unfortunately doesn't document this, but I assumed it's a static placeholder they just return. Why would/should the API ever return different number of asterisks?

btw. this is unrelated to how we treat sensitive data in the schema via Sensitive, it's more related to how the backend API (GitHub in this case) implements it.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, I think a code comment might be appropriate in this case mentioning it appears to be an undocumented GH api response.

if oldV == maskedSecret {
return true
}

return oldV == newV
},
},
"insecure_ssl": {
Type: schema.TypeString,
Optional: true,
},
},
},
},
"url": {
Type: schema.TypeString,
Expand Down Expand Up @@ -77,7 +110,11 @@ func resourceGithubRepositoryWebhookObject(d *schema.ResourceData) *github.Hook
URL: &url,
Events: events,
Active: &active,
Config: d.Get("configuration").(map[string]interface{}),
}

config := d.Get("configuration").([]interface{})
if len(config) > 0 {
hook.Config = config[0].(map[string]interface{})
}

return hook
Expand Down Expand Up @@ -115,7 +152,7 @@ func resourceGithubRepositoryWebhookRead(d *schema.ResourceData, meta interface{
d.Set("url", hook.URL)
d.Set("active", hook.Active)
d.Set("events", hook.Events)
d.Set("configuration", hook.Config)
d.Set("configuration", []interface{}{hook.Config})

return nil
}
Expand Down
83 changes: 83 additions & 0 deletions github/resource_github_repository_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,36 @@ func TestAccGithubRepositoryWebhook_basic(t *testing.T) {
})
}

func TestAccGithubRepositoryWebhook_secret(t *testing.T) {
randString := acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)
var hook github.Hook

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckGithubRepositoryWebhookDestroy,
Steps: []resource.TestStep{
{
Config: testAccGithubRepositoryWebhookConfig_secret(randString),
Check: resource.ComposeTestCheckFunc(
testAccCheckGithubRepositoryWebhookExists("github_repository_webhook.foo", fmt.Sprintf("foo-%s", randString), &hook),
testAccCheckGithubRepositoryWebhookAttributes(&hook, &testAccGithubRepositoryWebhookExpectedAttributes{
Name: "web",
Events: []string{"pull_request"},
Configuration: map[string]interface{}{
"url": "https://www.terraform.io/webhook",
"content_type": "json",
"secret": "********",
"insecure_ssl": "0",
},
Active: true,
}),
),
},
},
})
}

func TestAccGithubRepositoryWebhook_importBasic(t *testing.T) {
randString := acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)

Expand All @@ -80,6 +110,27 @@ func TestAccGithubRepositoryWebhook_importBasic(t *testing.T) {
})
}

func TestAccGithubRepositoryWebhook_importSecret(t *testing.T) {
randString := acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckGithubRepositoryDestroy,
Steps: []resource.TestStep{
{
Config: testAccGithubRepositoryWebhookConfig_secret(randString),
},
{
ResourceName: "github_repository_webhook.foo",
ImportState: true,
ImportStateVerify: true,
ImportStateIdPrefix: fmt.Sprintf("foo-%s/", randString),
},
},
})
}

func testAccCheckGithubRepositoryWebhookExists(n string, repoName string, hook *github.Hook) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[n]
Expand Down Expand Up @@ -196,6 +247,38 @@ func testAccGithubRepositoryWebhookConfig(randString string) string {
`, randString, randString)
}

func testAccGithubRepositoryWebhookConfig_secret(randString string) string {
return fmt.Sprintf(`
resource "github_repository" "foo" {
name = "foo-%s"
description = "Terraform acceptance tests"
homepage_url = "http://example.com/"

# So that acceptance tests can be run in a github organization
# with no billing
private = false

has_issues = true
has_wiki = true
has_downloads = true
}

resource "github_repository_webhook" "foo" {
repository = "${github_repository.foo.name}"

name = "web"
configuration {
url = "https://www.terraform.io/webhook"
content_type = "json"
secret = "RandomSecretString"
insecure_ssl = false
}

events = ["pull_request"]
}
`, randString)
}

func testAccGithubRepositoryWebhookUpdateConfig(randString string) string {
return fmt.Sprintf(`
resource "github_repository" "foo" {
Expand Down