zorkian / go-datadog-api

A Go implementation of the Datadog API.
BSD 3-Clause "New" or "Revised" License
183 stars 156 forks source link

Downtime Updates should update-in-place #240

Closed platinummonkey closed 5 years ago

platinummonkey commented 5 years ago

When updating downtimes, we should update the passed downtime struct when updating. Updates can do:

  1. Updating the downtime type
  2. Field sanitization

Also, in the future downtimes will be immutable and this is forwards compatible.

bkabrda commented 5 years ago

Hmm, so none of the other Update* methods do actually update the passed structure, but I guess this would be ok. Could you please add an integration test to integration/downtime_test.go to make sure this works right? Thanks!

platinummonkey commented 5 years ago

@bkabrda maybe dm me on public dd slack @Cody Lee the org name being used to run these integration tests and when we roll this out I'll update this in another PR for those TODO's

bkabrda commented 5 years ago

Thanks for updating the tests. I can run these myself and they pass, but there's a lot of typos, so here's a patch I'd like to ask you to apply before I can merge:

diff --git a/integration/downtime_test.go b/integration/downtime_test.go
index d905a41..f0f2a7a 100644
--- a/integration/downtime_test.go
+++ b/integration/downtime_test.go
@@ -70,18 +70,18 @@ func TestDowntimeUpdate(t *testing.T) {

        // uncomment once immutable to validate it changed to NotEqual
        assert.Equal(t, originalID, actual.GetId())
-       assert.Equal(t, downtime.GetActive(), acutal.GetActive())
-       assert.Equal(t, downtime.GetCancelled(), acutal.GetCancelled())
-       assert.Equal(t, downtime.GetDisabled(), acutal.GetDisabled())
-       assert.Equal(t, downtime.GetEnd(), acutal.GetEnd())
-       assert.Equal(t, downtime.GetMessage(), acutal.GetMessage())
-       assert.Equal(t, downtime.GetMonitorId(), acutal.GetMonitorId())
-       assert.Equal(t, downtime.GetMonitorTags(), acutal.GetMonitorTags())
-       assert.Equal(t, downtime.GetParentId(), acutal.GetParentId())
+       assert.Equal(t, downtime.GetActive(), actual.GetActive())
+       assert.Equal(t, downtime.GetCanceled(), actual.GetCanceled())
+       assert.Equal(t, downtime.GetDisabled(), actual.GetDisabled())
+       assert.Equal(t, downtime.GetEnd(), actual.GetEnd())
+       assert.Equal(t, downtime.GetMessage(), actual.GetMessage())
+       assert.Equal(t, downtime.GetMonitorId(), actual.GetMonitorId())
+       assert.Equal(t, downtime.MonitorTags, actual.MonitorTags)
+       assert.Equal(t, downtime.GetParentId(), actual.GetParentId())
        // timezone will be automatically updated to UTC
-       assert.Equal(t, "utc", actual.GetTimezone())
-       assert.Equal(t, downtime.GetRecurrence(), acutal.GetRecurrence())
-       assert.EqualValues(t, downtime.Scope, acutal.Scope)
+       assert.Equal(t, "UTC", actual.GetTimezone())
+       assert.Equal(t, downtime.GetRecurrence(), actual.GetRecurrence())
+       assert.EqualValues(t, downtime.Scope, actual.Scope)
        // in the future the replaced downtime will have an updated start time, flip this to NotEqual
        assert.Equal(t, downtime.GetStart(), actual.GetStart())
 }

Please also consider squashing the commits. Thanks!

bkabrda commented 5 years ago

LGTM now, merging. Thanks for the PR!