feat: allow a machine health check schedule of Never - #436
Open
Scott-Emberson wants to merge 2 commits into
Open
feat: allow a machine health check schedule of Never#436Scott-Emberson wants to merge 2 commits into
Scott-Emberson wants to merge 2 commits into
Conversation
FromTimeSpan read the time span fields at fixed offsets and took the day component from timeSpan[0:0], which is always the empty string. Every value carrying a day component therefore parsed to zero, including "1.00:00:00" — the interval on the default machine policy — and any fractional seconds were dropped. An empty string panicked on a slice bound. Parse the components by separator instead. The day and fractional-second parts are both optional, and the server does not pad the day component to a fixed width, so offsets cannot be assumed. Malformed input now yields a zero duration rather than a panic. The existing tests only logged their results and asserted nothing, which is why this went unnoticed; they now assert, and every case they already covered was returning zero. Closes OctopusDeploy#434 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Octopus server represents a health check schedule of "Never" as the absence of both HealthCheckInterval and HealthCheckCron; there is no schedule type on the wire. MarshalJSON always populated HealthCheckInterval through ToTimeSpan, and ToTimeSpan(0) is "00:00:00", which is not empty and so defeated the omitempty tag. No value of HealthCheckInterval could produce a payload without the field, leaving "Never" unreachable through this SDK. Write the interval only when it is non-zero, so a zero interval is omitted and the server stores null. UnmarshalJSON already leaves the field at zero when the server sends null, so the round trip is symmetric. Depends on the FromTimeSpan fix in the preceding commit: without it a policy whose interval carries a day component reads back as zero, and would then be written back as Never. Unblocks OctopusDeploy/terraform-provider-octopusdeploy#225 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unblocks OctopusDeploy/terraform-provider-octopusdeploy#225.
Stacked on #435, which needs to merge first. GitHub will not let me base a pull request from a fork on another fork branch, so the diff here shows both commits. Only the second one,
feat: allow a machine health check schedule of Never, is new.The problem
A health check schedule of "Never" is the absence of both
HealthCheckIntervalandHealthCheckCron. There is no schedule type on the wire; the portal derives it:MarshalJSONalways populated the interval throughToTimeSpan, andToTimeSpan(0)returns"00:00:00". That is not an empty string, so theomitemptytag on the wire struct never fired. No value ofHealthCheckIntervalcould produce a payload without the field, which left "Never" unreachable through this SDK, and"00:00:00"reads back as an interval of zero rather than as Never.The change
Write the interval only when it is non-zero.
UnmarshalJSONalready guards withif len(fields.HealthCheckInterval) > 0, so a null from the server leaves the field at zero and the round trip is symmetric.HealthCheckIntervalstays atime.Duration. Making it a pointer would express the same thing but break every caller that assigns to it.Applied to
pkg/machinepolicies/andpkg/machines/, which each carry their own copy.Why it depends on the other commit
Before the
FromTimeSpanfix, a policy whose interval carries a day component reads back as zero. Landing this change on its own would mean those policies get written back with the interval omitted, which switches them to Never."1.00:00:00"is the default machine policy's interval, so that would be most of them.Tests
Unit tests covering the omission at zero, the interval still being written when non-zero, and an absent interval deserialising to zero.
Verified against a 2026.x server through
machinepolicies.Addandmachinepolicies.Update: creating a policy with a zero interval storesHealthCheckInterval: null, re-reading gives0s, and updating a seven day policy to zero also lands as null. Reading the raw payload back confirms the field is absent rather than"00:00:00", so the portal shows the schedule as Never.