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

Accept "null" in monitor NoDataTimeframe #129

Merged
merged 2 commits into from Nov 28, 2017

Conversation

Projects
None yet
4 participants
@dtan4
Contributor

dtan4 commented Nov 6, 2017

Why

If the monitor is configured not to notify data missing, GetMonitor API returns null in no_data_timeframe field.
Therefore NoDataTimeframe.UnmarshalJSON tries to parse "null" as Int, then it returns error.

how to reproduce

package main

import (
	"os"

	"github.com/k0kubun/pp"
	"github.com/zorkian/go-datadog-api"
)

const (
	monitorID = 123456 // your monitor ID which does not notify data missing
)

func main() {
	apiKey, appKey := os.Getenv("DATADOG_API_KEY"), os.Getenv("DATADOG_APP_KEY")
	client := datadog.NewClient(apiKey, appKey)

	mon, err := client.GetMonitor(monitorID)
	if err != nil {
		panic(err)
	}

	pp.Println(mon)
}
$ go run hoge.go
panic: strconv.ParseInt: parsing "null": invalid syntax

goroutine 1 [running]:
main.main()
        /Users/dtan4/src/github.com/dtan4/terraforming-datadog/hoge.go:20 +0x1e1
exit status 2

What

Accept nil in no_data_timeframe and return empty value.

@philwhln

This comment has been minimized.

philwhln commented Nov 9, 2017

I'm hitting this issue too. This change makes sense to me.

@ojongerius ojongerius added the bug label Nov 10, 2017

@ojongerius

This comment has been minimized.

Collaborator

ojongerius commented Nov 12, 2017

@dtan4 awesome! Would you be able to add a test to /integration/monitors_test.go?

@fprimex

This comment has been minimized.

fprimex commented Nov 27, 2017

Hi, as mentioned over in the Terraform Datadog provider issues ( terraform-providers/terraform-provider-datadog#31 ), this PR would resolve an issue importing and converging resources when using Terraform. Is there a timeline for getting this merged? Is there any engineering effort that needs to be contributed to to get this accepted and released? Thanks!

@dtan4

This comment has been minimized.

Contributor

dtan4 commented Nov 28, 2017

Sorry for late! I added integration testcase to verify this change.

@ojongerius ojongerius merged commit a117c6a into zorkian:master Nov 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dtan4 dtan4 deleted the dtan4:accept-null-no-data-timeframe branch Nov 28, 2017

@fprimex

This comment has been minimized.

fprimex commented Nov 30, 2017

Awesome :) would it be possible to ping us in the TF issue that was linked when a release is made? I'll be keeping an eye out, but we'll probably do a new release immediately after it is available.

@ojongerius

This comment has been minimized.

Collaborator

ojongerius commented Dec 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment