feat: Add support for new runbook retention "Strategy" property #387
Conversation
|
|
There was a problem hiding this comment.
should we be including omitempty for strategy? I'd thought it was a required field. Is it because of backwards compatibility?
There was a problem hiding this comment.
You mean line 64 and line 81 in pkg/runbooks/runbook_retention_policy.go?
Yeah that is a mistake
|
|
||
| type RunbookRetentionPolicy struct { | ||
| Strategy string `json:"Strategy"` | ||
| QuantityToKeep int32 `json:"QuantityToKeep"` |
There was a problem hiding this comment.
Is QTK better as an "omitempty"?
There was a problem hiding this comment.
I am not marking it as omit empty for backwards compatibility. We want to serialise QTK and the unit for older servers that may not support the strategy field.
There was a problem hiding this comment.
ooooh this makes sense! very clever
| func NewDefaultRunbookRetentionPolicy() *RunbookRetentionPolicy { | ||
| return &RunbookRetentionPolicy{ | ||
| Strategy: RunbookRetentionStrategyDefault, | ||
| QuantityToKeep: 100, |
There was a problem hiding this comment.
If we are setting to a default strategy do we still need to set the QTK and unit?
There was a problem hiding this comment.
As mentioned above, keeping for backwards compatibility.
| func NewKeepForeverRunbookRetentionPolicy() *RunbookRetentionPolicy { | ||
| return &RunbookRetentionPolicy{ | ||
| Strategy: RunbookRetentionStrategyForever, | ||
| QuantityToKeep: 0, |
There was a problem hiding this comment.
can QTK and Unit be omitted? They will be ignored by the server. Or is it needed for compatibility with the current terraform for runbooks?
There was a problem hiding this comment.
As mentioned above, keeping for backwards compatibility.
| } | ||
|
|
||
| if fields.QuantityToKeep == 100 && r.Unit == RunbookRetentionUnitItems { | ||
| r.Strategy = RunbookRetentionStrategyDefault |
There was a problem hiding this comment.
- If the server is old and doesn't support default but the go client is new, what will happen?
- If the user changed the default strategy to QTK = 50 could they send an API request with omitted Strategy, QTK = 0 and Unit = Items and have unexpected results?
There was a problem hiding this comment.
If the server is old and doesn't support default but the go client is new, what will happen?
If the server is old and the Go client is new, since the Go client is still sending the old property, the server will receive the system default properties. Then, when the server is upgraded, the values will be correctly migrated to the default value.
If the user changed the default strategy to QTK = 50 could they send an API request with omitted Strategy, QTK = 0 and Unit = Items and have unexpected results?
Not sure if I follow. I think what you mean is: Would the user experience unexpected results if they omit the strategy and send the system default value, but they are on the new server and has set a space-wide default?
If the user sets the policy with the system default (QTK = 100 + Unit = items) and doesn't set the strategy, it will set the policy to the space default.
I think this is fine since the number of users that do this will be small compared to the alternative, where a bunch of runbooks will get custom values when the user is not intending to.
There was a problem hiding this comment.
It's worth noting that they will have to update their policy to deliberately omit the strategy policy. Since the default value for their runbooks will have the new "strategy" property.
Rose-Northey
left a comment
Rose-Northey
left a comment
There was a problem hiding this comment.
Hey, looking good but I left some questions on the approach. I'd love to discuss
octopus-hideaki
changed the title
Rose-Northey
left a comment
Rose-Northey
left a comment
There was a problem hiding this comment.
Cheers for the clarifications! Thanks heaps
2 participants
Add support for the new "Strategy" property in Octopus server.
[sc-126260]
This enables runbook retention to be set to: