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

Downtime Updates should update-in-place #240

Merged
merged 1 commit into from
May 17, 2019

Conversation

platinummonkey
Copy link
Contributor

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
Copy link
Collaborator

bkabrda commented May 16, 2019

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
Copy link
Contributor Author

@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
Copy link
Collaborator

bkabrda commented May 17, 2019

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!

this contains the downtime type information along with potentially sanitizing replacing fields

in the future downtimes will be immutable and this is forwards compatible
@bkabrda
Copy link
Collaborator

bkabrda commented May 17, 2019

LGTM now, merging. Thanks for the PR!

@bkabrda bkabrda merged commit 5ee52bf into zorkian:master May 17, 2019
@platinummonkey platinummonkey deleted the downtime-updates-replace branch May 17, 2019 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants