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

Use `json.Number' for TTLs and fix Makefile #83

Closed
wants to merge 1 commit into from

Conversation

@clickyotomy
Copy link
Contributor

clickyotomy commented Nov 20, 2019

Hello,

I tried to run the example snippet from the README with a slight modification, and it doesn't seem to work because of JSON parse errors.

  • Go Version
go version go1.13.4 darwin/amd64
  • Snippet
package main

import (
    "fmt"
    "net/http"
    "time"

    api "gopkg.in/ns1/ns1-go.v2/rest"
)

func main() {
    k := "{api-key}"
    httpClient := &http.Client{Timeout: time.Second * 10}
    client := api.NewClient(httpClient, api.SetAPIKey(k))
    zone, _, err := client.Zones.Get("example.com")

    if err != nil {
        fmt.Println(err)
        return
    }

    for _, r := range(zone.Records) {
        fmt.Println(r)
    }
}
  • Output
json: cannot unmarshal number 60.0 into Go struct field Zone.ttl of type int

This PR changes the .*TTL fields from type int to json.Number, for which .Float64, .Int64 methods can be used to get the actual values later on.

Also made a change to the Makefile to go fmt the files in the directory (please let me know if that should be in a different PR).

@rupa

This comment has been minimized.

Copy link
Contributor

rupa commented Nov 20, 2019

Hi, thanks for the PR! Nice and welcome catch on the go fmt stuff.

I haven't looked at it in detail yet, but the type change looks like it could break existing uses, so it's unlikely to be accepted, at least right away. Looks like we should at least update the documentation though, and it seems likely we could improve things a bit and keep things very back-compatible.

If you want to separate out the go fmt changes though, I'll happily get that merged right away!

@clickyotomy

This comment has been minimized.

Copy link
Contributor Author

clickyotomy commented Nov 20, 2019

@rupa - Thanks for taking a look. Yes, it looks like this change might break a few things, including terraform-provider-ns1. I'll close this out, and maybe we can re-open this some other time (if need be).

Opened #85 for the Makefile stuff.

@rupa

This comment has been minimized.

Copy link
Contributor

rupa commented Nov 21, 2019

@clickyotomy one thing to mention - I couldn't replicate this issue with my own setup - the TTLs should be ints. If you've found a repeatable way to get the TTL set/responding as e.g. 60.0 that could be something we'd want to fix :)

@clickyotomy

This comment has been minimized.

Copy link
Contributor Author

clickyotomy commented Nov 23, 2019

@rupa - Welp, it looks like there might be a problem with the response returned from the API itself. I checked the string response of the body in client.Do, before it was passed to json.Decoder.Decode and it looks like the API returned the value 60.0 for ttl. The UI on the NS1 control panel just says "60" for the SOA TTL text box. Not sure why the response returned from the API has 60.0

Response (removed/modified some fields):

{
   "nx_ttl":3600,
   "retry":7200,
   "zone":"example.com",
   "network_pools":[
      "pN"
   ],
   "serial":1234567890,
   "primary":{
      "enabled":false,
      "secondaries":[

      ]
   },
   "refresh":43200,
   "expiry":1209600,
   "dns_servers":[
      "foo.nsone.net",
      "bar.nsone.net",
   ],
   "records":["{removed}"],
   "meta":{
   },
   "primary_master":"foo.nsone.net",
   "ttl":60.0,
   "id":"some_id",
   "hostmaster":"hostmaster@nsone.net",
   "networks":[
      0
   ],
   "pool":"pN"
}
@rupa

This comment has been minimized.

Copy link
Contributor

rupa commented Nov 25, 2019

yep, you're right. I did some sleuthing and my suspicion is that this record may have been created or updated before some of the current validation/coercion was in place. it might be worth contacting support so they can take a look at your account specifically and ensure everything is squared up going forward (or to nail down specifics if there's any issue with current behavior). I've also created a ticket for us to take a look at this (potential) issue in general, but I don't have a specific timeline on that.\

thanks again!

@clickyotomy

This comment has been minimized.

Copy link
Contributor Author

clickyotomy commented Nov 26, 2019

Will do, thanks for checking! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.