Skip to content

Commit

Permalink
Respond to review feedback
Browse files Browse the repository at this point in the history
wchrisjohnson committed May 25, 2017

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent 6a0dca9 commit 1c76194
Showing 3 changed files with 109 additions and 135 deletions.
2 changes: 2 additions & 0 deletions builtin/providers/librato/common_helpers_test.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
package librato

import (
"log"
"testing"
"time"
)

func sleep(t *testing.T, amount time.Duration) func() {
return func() {
log.Printf("[INFO] Sleeping for %d seconds...", amount)
time.Sleep(amount * time.Second)
}
}
190 changes: 76 additions & 114 deletions builtin/providers/librato/resource_librato_metric.go
Original file line number Diff line number Diff line change
@@ -4,19 +4,18 @@ import (
"encoding/json"
"fmt"
"log"
"reflect"
"time"

"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/helper/schema"
"github.com/henrikhodne/go-librato/librato"
)

const (
metricTypeGauge = "gauge"
metricTypeCounter = "counter"
metricTypeComposite = "composite"
)
// const (
// metricTypeCounter = "counter"
// metricTypeGauge = "gauge"
// metricTypeComposite = "composite"
// )

func resourceLibratoMetric() *schema.Resource {
return &schema.Resource{
@@ -26,69 +25,71 @@ func resourceLibratoMetric() *schema.Resource {
Delete: resourceLibratoMetricDelete,

Schema: map[string]*schema.Schema{
"name": &schema.Schema{
"name": {
Type: schema.TypeString,
Required: true,
ForceNew: false,
},
"type": &schema.Schema{
"type": {
Type: schema.TypeString,
Required: true,
},
"display_name": &schema.Schema{
"display_name": {
Type: schema.TypeString,
Optional: true,
},
"description": &schema.Schema{
"description": {
Type: schema.TypeString,
Optional: true,
},
"period": &schema.Schema{
"period": {
Type: schema.TypeInt,
Optional: true,
},
"composite": &schema.Schema{
"composite": {
Type: schema.TypeString,
Optional: true,
},
"attributes": &schema.Schema{
"attributes": {
Type: schema.TypeList,
Optional: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"color": &schema.Schema{
"color": {
Type: schema.TypeString,
Optional: true,
},
"display_max": &schema.Schema{
"display_max": {
Type: schema.TypeString,
Optional: true,
},
"display_min": &schema.Schema{
"display_min": {
Type: schema.TypeString,
Optional: true,
},
"display_units_long": &schema.Schema{
"display_units_long": {
Type: schema.TypeString,
Optional: true,
},
"display_units_short": &schema.Schema{
"display_units_short": {
Type: schema.TypeString,
Optional: true,
},
"display_stacked": &schema.Schema{
"display_stacked": {
Type: schema.TypeBool,
Optional: true,
Default: false,
},
"created_by_ua": &schema.Schema{
"created_by_ua": {
Type: schema.TypeString,
Computed: true,
},
"gap_detection": &schema.Schema{
"gap_detection": {
Type: schema.TypeBool,
Optional: true,
},
"aggregate": &schema.Schema{
"aggregate": {
Type: schema.TypeBool,
Optional: true,
},
@@ -102,21 +103,18 @@ func resourceLibratoMetric() *schema.Resource {
func resourceLibratoMetricCreate(d *schema.ResourceData, meta interface{}) error {
client := meta.(*librato.Client)

metric := new(librato.Metric)
if a, ok := d.GetOk("name"); ok {
metric.Name = librato.String(a.(string))
metric := librato.Metric{
Name: librato.String(d.Get("name").(string)),
Type: librato.String(d.Get("type").(string)),
}
if a, ok := d.GetOk("display_name"); ok {
metric.DisplayName = librato.String(a.(string))
}
if a, ok := d.GetOk("type"); ok {
metric.Type = librato.String(a.(string))
}
if a, ok := d.GetOk("description"); ok {
metric.Description = librato.String(a.(string))
}
if a, ok := d.GetOk("period"); ok {
metric.Period = librato.Uint(a.(uint))
metric.Period = librato.Uint(uint(a.(int)))
}
if a, ok := d.GetOk("composite"); ok {
metric.Composite = librato.String(a.(string))
@@ -125,14 +123,6 @@ func resourceLibratoMetricCreate(d *schema.ResourceData, meta interface{}) error
if a, ok := d.GetOk("attributes"); ok {

attributeData := a.([]interface{})
if len(attributeData) > 1 {
return fmt.Errorf("Only one set of attributes per metric is supported")
}

if len(attributeData) == 1 && attributeData[0] == nil {
return fmt.Errorf("No attributes found in attributes block")
}

attributeDataMap := attributeData[0].(map[string]interface{})
attributes := new(librato.MetricAttributes)

@@ -167,13 +157,13 @@ func resourceLibratoMetricCreate(d *schema.ResourceData, meta interface{}) error
metric.Attributes = attributes
}

_, err := client.Metrics.Edit(metric)
_, err := client.Metrics.Edit(&metric)
if err != nil {
log.Printf("[INFO] ERROR creating Metric: %s", err)
return fmt.Errorf("Error creating Librato metric: %s", err)
}

resource.Retry(1*time.Minute, func() *resource.RetryError {
retryErr := resource.Retry(1*time.Minute, func() *resource.RetryError {
_, _, err := client.Metrics.Get(*metric.Name)
if err != nil {
if errResp, ok := err.(*librato.ErrorResponse); ok && errResp.Response.StatusCode == 404 {
@@ -183,8 +173,12 @@ func resourceLibratoMetricCreate(d *schema.ResourceData, meta interface{}) error
}
return nil
})
if retryErr != nil {
return fmt.Errorf("Error creating Librato metric: %s", retryErr)
}

return metricReadResult(d, metric)
d.SetId(*metric.Name)
return resourceLibratoMetricRead(d, meta)
}

func resourceLibratoMetricRead(d *schema.ResourceData, meta interface{}) error {
@@ -202,60 +196,61 @@ func resourceLibratoMetricRead(d *schema.ResourceData, meta interface{}) error {
return fmt.Errorf("Error reading Librato Metric %s: %s", id, err)
}

log.Printf("[INFO] Read Librato Metric: %s", structToString(metric))
d.Set("name", metric.Name)
d.Set("type", metric.Type)

return metricReadResult(d, metric)
}
if metric.Description != nil {
d.Set("description", metric.Description)
}

func resourceLibratoMetricUpdate(d *schema.ResourceData, meta interface{}) error {
client := meta.(*librato.Client)
if metric.DisplayName != nil {
d.Set("display_name", metric.DisplayName)
}

if metric.Period != nil {
d.Set("period", metric.Period)
}

metricID := d.Id()
if metric.Composite != nil {
d.Set("composite", metric.Composite)
}

// Just to have whole object for comparison before/after update
fullMetric, _, err := client.Metrics.Get(metricID)
if err != nil {
attributes := metricAttributesGather(d, metric.Attributes)

// Since attributes isn't a simple terraform type (TypeList), it's best to
// catch the error returned from the d.Set() function, and handle accordingly.
if err := d.Set("attributes", attributes); err != nil {
return err
}

return nil
}

func resourceLibratoMetricUpdate(d *schema.ResourceData, meta interface{}) error {
client := meta.(*librato.Client)

id := d.Id()

metric := new(librato.Metric)
metric.Name = librato.String(d.Get("name").(string))
metric.Name = librato.String(id)

if d.HasChange("type") {
metric.Type = librato.String(d.Get("type").(string))
fullMetric.Type = metric.Type
}

if d.HasChange("description") {
metric.Description = librato.String(d.Get("description").(string))
fullMetric.Description = metric.Description
}

if d.HasChange("display_name") {
metric.DisplayName = librato.String(d.Get("display_name").(string))
fullMetric.DisplayName = metric.DisplayName
}

if d.HasChange("period") {
metric.Period = librato.Uint(d.Get("period").(uint))
fullMetric.Period = metric.Period
metric.Period = librato.Uint(uint(d.Get("period").(int)))
}

if d.HasChange("composite") {
metric.Composite = librato.String(d.Get("composite").(string))
fullMetric.Composite = metric.Composite
}

if d.HasChange("attributes") {
attributeData := d.Get("attributes").([]interface{})
if len(attributeData) > 1 {
return fmt.Errorf("Only one set of attributes per metric is supported")
}

if len(attributeData) == 1 && attributeData[0] == nil {
return fmt.Errorf("No attributes found in attributes block")
}

attributeDataMap := attributeData[0].(map[string]interface{})
attributes := new(librato.MetricAttributes)

@@ -286,20 +281,17 @@ func resourceLibratoMetricUpdate(d *schema.ResourceData, meta interface{}) error
if v, ok := attributeDataMap["aggregate"].(bool); ok {
attributes.Aggregate = *librato.Bool(v)
}

metric.Attributes = attributes
fullMetric.Attributes = attributes
}

log.Printf("[INFO] Updating Librato metric: %v", structToString(metric))
log.Printf("[INFO] Librato fullMetric: %v", structToString(fullMetric))

_, err = client.Metrics.Edit(metric)
_, err := client.Metrics.Edit(metric)
if err != nil {
return fmt.Errorf("Error updating Librato metric: %s", err)
}

log.Printf("[INFO] Updated Librato metric %s", metricID)
log.Printf("[INFO] Updated Librato metric %s", id)

// Wait for propagation since Librato updates are eventually consistent
wait := resource.StateChangeConf{
@@ -309,21 +301,19 @@ func resourceLibratoMetricUpdate(d *schema.ResourceData, meta interface{}) error
MinTimeout: 2 * time.Second,
ContinuousTargetOccurence: 5,
Refresh: func() (interface{}, string, error) {
log.Printf("[INFO] Checking if Librato Metric %s was updated yet", metricID)
changedMetric, _, getErr := client.Metrics.Get(metricID)
log.Printf("[INFO] Checking if Librato Metric %s was updated yet", id)
changedMetric, _, getErr := client.Metrics.Get(id)
if getErr != nil {
return changedMetric, "", getErr
}
isEqual := reflect.DeepEqual(*fullMetric, *changedMetric)
log.Printf("[INFO] Updated Librato Metric %s match: %t", metricID, isEqual)
return changedMetric, fmt.Sprintf("%t", isEqual), nil
return changedMetric, "true", nil
},
}

_, err = wait.WaitForState()
if err != nil {
log.Printf("[INFO] ERROR - Failed updating Librato Metric %s: %s", metricID, err)
return fmt.Errorf("Failed updating Librato Metric %s: %s", metricID, err)
log.Printf("[INFO] ERROR - Failed updating Librato Metric %s: %s", id, err)
return fmt.Errorf("Failed updating Librato Metric %s: %s", id, err)
}

return resourceLibratoMetricRead(d, meta)
@@ -341,13 +331,13 @@ func resourceLibratoMetricDelete(d *schema.ResourceData, meta interface{}) error
}

log.Printf("[INFO] Verifying Metric %s deleted", id)
resource.Retry(1*time.Minute, func() *resource.RetryError {
retryErr := resource.Retry(1*time.Minute, func() *resource.RetryError {

log.Printf("[INFO] GETing Metric %s", id)
log.Printf("[INFO] Getting Metric %s", id)
_, _, err := client.Metrics.Get(id)
if err != nil {
if errResp, ok := err.(*librato.ErrorResponse); ok && errResp.Response.StatusCode == 404 {
log.Printf("[INFO] GET returned a 404 for Metric %s\n", id)
log.Printf("[INFO] Metric %s not found, removing from state", id)
return nil
}
log.Printf("[INFO] non-retryable error attempting to Get metric: %s", err)
@@ -357,38 +347,10 @@ func resourceLibratoMetricDelete(d *schema.ResourceData, meta interface{}) error
log.Printf("[INFO] retryable error attempting to Get metric: %s", id)
return resource.RetryableError(fmt.Errorf("metric still exists"))
})

log.Println("[INFO] I think Metric is deleted")

d.SetId("")
return nil
}

func metricReadResult(d *schema.ResourceData, metric *librato.Metric) error {
d.SetId(*metric.Name)
d.Set("id", *metric.Name)
d.Set("name", *metric.Name)
d.Set("type", *metric.Type)

if metric.Description != nil {
d.Set("description", *metric.Description)
}

if metric.DisplayName != nil {
d.Set("display_name", *metric.DisplayName)
if retryErr != nil {
return fmt.Errorf("Error deleting librato metric: %s", retryErr)
}

if metric.Period != nil {
d.Set("period", *metric.Period)
}

if metric.Composite != nil {
d.Set("composite", *metric.Composite)
}

attributes := metricAttributesGather(d, metric.Attributes)
d.Set("attributes", attributes)

return nil
}

52 changes: 31 additions & 21 deletions builtin/providers/librato/resource_librato_metric_test.go
Original file line number Diff line number Diff line change
@@ -11,18 +11,20 @@ import (
"github.com/henrikhodne/go-librato/librato"
)

func TestAccLibratoCounterMetric(t *testing.T) {
func TestAccLibratoMetrics(t *testing.T) {
var metric librato.Metric

name := fmt.Sprintf("tftest-metric-%s", acctest.RandString(10))
typ := "counter"

desc1 := fmt.Sprintf("A test %s metric", typ)
desc2 := fmt.Sprintf("An updated test %s metric", typ)
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckLibratoMetricDestroy,
Steps: []resource.TestStep{
{
Config: counterMetricConfig(name, typ, fmt.Sprintf("A test %s metric", typ)),
Config: counterMetricConfig(name, typ, desc1),
Check: resource.ComposeTestCheckFunc(
testAccCheckLibratoMetricExists("librato_metric.foobar", &metric),
testAccCheckLibratoMetricName(&metric, name),
@@ -33,31 +35,30 @@ func TestAccLibratoCounterMetric(t *testing.T) {
},
{
PreConfig: sleep(t, 15),
Config: counterMetricConfig(name, typ, fmt.Sprintf("An updated test %s metric", typ)),
Config: counterMetricConfig(name, typ, desc2),
Check: resource.ComposeTestCheckFunc(
testAccCheckLibratoMetricExists("librato_metric.foobar", &metric),
testAccCheckLibratoMetricName(&metric, name),
testAccCheckLibratoMetricType(&metric, typ),
testAccCheckLibratoMetricDescription(&metric, desc2),
resource.TestCheckResourceAttr(
"librato_metric.foobar", "name", name),
),
},
},
})
}

func TestAccLibratoGaugeMetric(t *testing.T) {
var metric librato.Metric
name := fmt.Sprintf("tftest-metric-%s", acctest.RandString(10))
typ := "gauge"

name = fmt.Sprintf("tftest-metric-%s", acctest.RandString(10))
typ = "gauge"
desc1 = fmt.Sprintf("A test %s metric", typ)
desc2 = fmt.Sprintf("An updated test %s metric", typ)
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckLibratoMetricDestroy,
Steps: []resource.TestStep{
{
Config: gaugeMetricConfig(name, typ, fmt.Sprintf("A test %s metric", typ)),
Config: gaugeMetricConfig(name, typ, desc1),
Check: resource.ComposeTestCheckFunc(
testAccCheckLibratoMetricExists("librato_metric.foobar", &metric),
testAccCheckLibratoMetricName(&metric, name),
@@ -68,31 +69,30 @@ func TestAccLibratoGaugeMetric(t *testing.T) {
},
{
PreConfig: sleep(t, 15),
Config: gaugeMetricConfig(name, typ, fmt.Sprintf("An updated test %s metric", typ)),
Config: gaugeMetricConfig(name, typ, desc2),
Check: resource.ComposeTestCheckFunc(
testAccCheckLibratoMetricExists("librato_metric.foobar", &metric),
testAccCheckLibratoMetricName(&metric, name),
testAccCheckLibratoMetricType(&metric, typ),
testAccCheckLibratoMetricDescription(&metric, desc2),
resource.TestCheckResourceAttr(
"librato_metric.foobar", "name", name),
),
},
},
})
}

func TestAccLibratoCompositeMetric(t *testing.T) {
var metric librato.Metric
name := fmt.Sprintf("tftest-metric-%s", acctest.RandString(10))
typ := "composite"

name = fmt.Sprintf("tftest-metric-%s", acctest.RandString(10))
typ = "composite"
desc1 = fmt.Sprintf("A test %s metric", typ)
desc2 = fmt.Sprintf("An updated test %s metric", typ)
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckLibratoMetricDestroy,
Steps: []resource.TestStep{
{
Config: compositeMetricConfig(name, typ, fmt.Sprintf("A test %s metric", typ)),
Config: compositeMetricConfig(name, typ, desc1),
Check: resource.ComposeTestCheckFunc(
testAccCheckLibratoMetricExists("librato_metric.foobar", &metric),
testAccCheckLibratoMetricName(&metric, name),
@@ -103,11 +103,12 @@ func TestAccLibratoCompositeMetric(t *testing.T) {
},
{
PreConfig: sleep(t, 15),
Config: compositeMetricConfig(name, typ, fmt.Sprintf("An updated test %s metric", typ)),
Config: compositeMetricConfig(name, typ, desc2),
Check: resource.ComposeTestCheckFunc(
testAccCheckLibratoMetricExists("librato_metric.foobar", &metric),
testAccCheckLibratoMetricName(&metric, name),
testAccCheckLibratoMetricType(&metric, typ),
testAccCheckLibratoMetricDescription(&metric, desc2),
resource.TestCheckResourceAttr(
"librato_metric.foobar", "name", name),
),
@@ -124,7 +125,6 @@ func testAccCheckLibratoMetricDestroy(s *terraform.State) error {
continue
}

fmt.Printf("*** testAccCheckLibratoMetricDestroy ID: %s\n", rs.Primary.ID)
_, _, err := client.Metrics.Get(rs.Primary.ID)
if err == nil {
return fmt.Errorf("Metric still exists")
@@ -144,6 +144,16 @@ func testAccCheckLibratoMetricName(metric *librato.Metric, name string) resource
}
}

func testAccCheckLibratoMetricDescription(metric *librato.Metric, desc string) resource.TestCheckFunc {
return func(s *terraform.State) error {
if metric.Description == nil || *metric.Description != desc {
return fmt.Errorf("Bad description: %s", *metric.Description)
}

return nil
}
}

func testAccCheckLibratoMetricType(metric *librato.Metric, wantType string) resource.TestCheckFunc {
return func(s *terraform.State) error {
if metric.Type == nil || *metric.Type != wantType {

0 comments on commit 1c76194

Please sign in to comment.