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

[slos] fix API payloads #252

Merged
merged 6 commits into from
Aug 5, 2019
Merged

Conversation

platinummonkey
Copy link
Contributor

@platinummonkey platinummonkey commented Jul 23, 2019

We changed the name of this the threshold slo property to target as it's more meaningful. The hardest part of SW - naming.

Also updated the content types of create and added request validation as well.

@bkabrda
Copy link
Collaborator

bkabrda commented Jul 29, 2019

The recent changes have removed the createSLOThreshold and createUpdateServiceLevelObjective, but (some of) the accessors for these remained in the autogenerated accessors file for these, so the compilation fails. Could you please remove these so I can test and merge? Thanks!

@platinummonkey
Copy link
Contributor Author

yup! I'm waiting for us to merge && deploy the final API changes before continuing this work 👌

@platinummonkey platinummonkey changed the title [slos] fix threshold name [slos] fix API payloads Jul 30, 2019
@bkabrda
Copy link
Collaborator

bkabrda commented Jul 31, 2019

@platinummonkey ok, feel free to ping me whenever this is ready to be re-reviewed.

@platinummonkey
Copy link
Contributor Author

@bkabrda good to go 👍

--- PASS: TestServiceLevelObjectivesCreateGetUpdateAndDelete (3.84s)
PASS

Copy link
Collaborator

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

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

Looks great, I just have one question in comment inline. Once that's clear, I can go forward and merge this.

@@ -34,32 +35,38 @@ func TestServiceLevelObjectivesCreateGetUpdateAndDelete(t *testing.T) {
assert.Equal(t, expected.Description, actual.Description)
assert.True(t, expected.Thresholds.Equal(actual.Thresholds))

time.Sleep(time.Second)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the sleep? Is it expected that the SLO object is not available through API immediately? Or is it available but incomplete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that in a few rare cases, the read replicas were behind just enough to make this fail without some artificial delay.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, thanks for the explanation.

@bkabrda
Copy link
Collaborator

bkabrda commented Aug 5, 2019

LGTM now, merging. Thanks!

@bkabrda bkabrda merged commit 93f907f into zorkian:master Aug 5, 2019
@platinummonkey platinummonkey deleted the slo-api-fixes branch August 5, 2019 15:59
jbenais pushed a commit to jbenais/go-datadog-api that referenced this pull request Aug 8, 2019
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