-
Notifications
You must be signed in to change notification settings - Fork 23
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
refactor!: use pointers in struct fields #90
Conversation
In a struct context, these functions will be useful for distinguishing between unset fields and those set to a zero-value.
Failing to set this field makes ThousandEyes assigning default alert rules, which confuses the Terraform state. Better to be explicit and assign alert rules via Terraform.
DNS+ is no longer supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good left a couple of questions
AlertsEnabled int `json:"alertsEnabled,omitempty"` | ||
AlertRules []AlertRule `json:"alertRules,omitempty"` | ||
AlertsEnabled *int `json:"alertsEnabled,omitempty"` | ||
AlertRules []AlertRule `json:"alertRules"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was the omitempty
removed? Applies to other struct with the AlertRules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review, Sergio.
I mentioned this change in the description of the PR:
I'm dropping omitempty for alertRules. Failing to set this field makes ThousandEyes assign default alert rules, which confuses the Terraform state. Better to be explicit and assign alert rules via Terraform.
|
||
// Bool is a helper routine that allocates a new bool value | ||
// to store v and returns a pointer to it. | ||
func Bool(v bool) *bool { return &v } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a common practice in Go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it's common, but I think it makes sense for our use-case. I took it verbatim from Google's go-github API wrapper library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments.
} | ||
|
||
// extractPort - Set Server and Port fields if they are combined in the Server field. | ||
func extractPort(test AgentServer) (AgentServer, error) { | ||
// Unfortunately, the V6 API returns the server value with the port, | ||
// rather than having them in separate values as the API requires for | ||
// submissions. Not required for ICMP tests. | ||
var err error | ||
if test.Protocol != "ICMP" && strings.Index(test.Server, ":") != -1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a suggestion, if you cannot have an empty protocol, then you could define a method getProtocol()
which would return the protocol or empty if null; perhaps, this would simplify these conditions, but it may not be worth it if it's just here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion. It seems that only here we're dealing with this case. I can address this on another pull request.
} | ||
|
||
// Notification - Alert Rule Notification structure | ||
type Notification struct { | ||
Email NotificationEmail `json:"email,omitempty"` | ||
Email NotificationEmail `json:"email,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a pointer as well to make the omitempty
meaningful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point. On this contribution I focused on string
and int
types. Based on some tests I did, it seems we can avoid doing "pointerizing" slice
types. That leave us with struct types. I believe we'll need to make the change on those as well. I'll do that on a new pull request.
@@ -60,10 +60,13 @@ func (c *Client) GetGroupLabel(id int) (*GroupLabel, error) { | |||
|
|||
// CreateGroupLabel - Create label | |||
func (c Client) CreateGroupLabel(a GroupLabel) (*GroupLabel, error) { | |||
path := fmt.Sprintf("/groups/%s/new", a.Type) | |||
if a.Type == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, a GetType()
method could simplify this code.
http_server.go
Outdated
Type string `json:"type,omitempty"` | ||
// LiveShare is common to all tests except DNS+ | ||
LiveShare int `json:"liveShare,omitempty"` | ||
TestID *int64 `json:"testId,omitempty,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
omitempty
seems repeated here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, good catch. Fixed.
I'll merge and apply the recommendations on a new pull request. This one is already too bloated. |
This pull request is the first of a set of refactories with breaking changes. The objective is mitigate a class of bugs (for example) where the JSON marshaling fails to distinguish between unset fields and those set to a zero-value. One solution is to use pointers for struct fields. This concept isn't novel, I've taken it from the go-github wrapper.
Additionally, I'm including two other changes:
TestID
is now int64. In general, I want to future-proof our IDs to support > 2^32. I'll prepare another pull request modifying the rest of the ID fields.omitempty
foralertRules
. Failing to set this field makes ThousandEyes assign default alert rules, which confuses the Terraform state. Better to be explicit and assign alert rules via Terraform.All tests are passing after these changes: