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

Add additional arguments to Downtime struct #227

Merged
merged 3 commits into from
Apr 18, 2019
Merged

Add additional arguments to Downtime struct #227

merged 3 commits into from
Apr 18, 2019

Conversation

vanvlack
Copy link
Contributor

re: #226

Adds additional arguments to the Downtime struct

@vanvlack
Copy link
Contributor Author

@ojongerius do you have a chance for review?

@bkabrda
Copy link
Collaborator

bkabrda commented Apr 2, 2019

@vanvlack hey, is there any reason why there are no getters/setters for Downtime.MonitorTags? This PR looks great otherwise BTW!

@vanvlack
Copy link
Contributor Author

vanvlack commented Apr 9, 2019

is there any reason why there are no getters/setters for Downtime.MonitorTags

Unsure, will try to regenerate the get/set stuff and see what happens.

@vanvlack
Copy link
Contributor Author

vanvlack commented Apr 9, 2019

@bkabrda by the looks, no arrays (even ones elsewhere) are in the generated get/set file.

Copy link
Collaborator

@ojongerius ojongerius left a comment

Choose a reason for hiding this comment

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

Apart from adding ParentId and TimeZone to the test this looks great 👍

Active: datadog.Bool(false),
Disabled: datadog.Bool(false),
Message: datadog.String("Test downtime message"),
MonitorTags: []string{"some:tag"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please add ParentId too?

Disabled: datadog.Bool(false),
Message: datadog.String("Test downtime message"),
MonitorTags: []string{"some:tag"},
Scope: []string{"env:downtime_test"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah and TimeZone

@bkabrda
Copy link
Collaborator

bkabrda commented Apr 10, 2019

@vanvlack ah, right, the getters and setters are only generated for struct members that are pointers, so everything is correct in this case.

@bkabrda
Copy link
Collaborator

bkabrda commented Apr 16, 2019

Hey @vanvlack if you could address comments from @ojongerius and rebase, that would be awesome and also allow me to merge this PR.

@vanvlack
Copy link
Contributor Author

@bkabrda updated

@bkabrda
Copy link
Collaborator

bkabrda commented Apr 18, 2019

Ok, the tests are passing and all comments that @ojongerius made were addressed. LGTM, merging. Thanks a lot for the PR!

@bkabrda bkabrda dismissed ojongerius’s stale review April 18, 2019 05:41

All the comments were addressed in an additional commit.

@bkabrda bkabrda merged commit 081feaa into zorkian:master Apr 18, 2019
@vanvlack vanvlack deleted the datadog/downtime-tags branch April 22, 2019 17:17
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.

3 participants