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

Should all fields in Service type be `omitempty #348

Closed
callumj opened this issue Aug 18, 2021 · 4 comments · Fixed by #384
Closed

Should all fields in Service type be `omitempty #348

callumj opened this issue Aug 18, 2021 · 4 comments · Fixed by #384
Milestone

Comments

@callumj
Copy link

callumj commented Aug 18, 2021

I am trying to just change the alert_creation type on a service (via UpdateService) and have noticed I cannot just set it as the JSON marshal wants to include auto_resolve_timeout, acknowledgement_timeout, escalation_policy (it isn't a pointer so omitempty doesn't work), support_hours, scheduled_actions.

Is there a reason a few of these are lacking omitempty?

@theckman
Copy link
Collaborator

@callumj for EscalationPolicy, the API reference documentation indicates it's required when updating a service. So I think that will need to stay as-is.

For the other fields it seems like more of an accidental omission within the package. We should probably fix those with the similar issues we're hoping to address in v1.5.0.

@theckman theckman added this to the v1.5.0 milestone Aug 30, 2021
@theckman
Copy link
Collaborator

Change of plans, going to fix this in v1.4.2 because it broke some things.

@theckman theckman modified the milestones: v1.5.0, v1.4.2 Aug 30, 2021
@theckman theckman added bug and removed enhancement labels Aug 30, 2021
@theckman
Copy link
Collaborator

Slight correction, I'll fix the things that broke in v1.4.2. The rest will be done in v.1.5.0.

@theckman theckman modified the milestones: v1.4.2, v1.5.0 Aug 30, 2021
@theckman theckman added enhancement and removed bug labels Aug 30, 2021
theckman added a commit that referenced this issue Aug 30, 2021
The lack of `omitempty` on these fields is causing `CreateService` calls to fail
when both are omitted. This broke in the v1.4.0 release so this is a bug fix.

Relates to #348

Fixes #346
@theckman
Copy link
Collaborator

theckman commented Sep 1, 2021

@callumj Actually, would you be wiling to raise a PR with omitempty added to all fields except EscalationPolicy?

Because of how the repo is configured, I can't merge my own PRs without getting a review from a PD employee when they have a spare moment. Those spare cycles can be hard to come by, unfortunately.

I can review and merge the PRs of others, and so if you could raise a PR, and have a moment to address any feedback, it would be a tremendous help in getting this change made.

If not no sweat!

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

Successfully merging a pull request may close this issue.

2 participants