-
Notifications
You must be signed in to change notification settings - Fork 26
feat: Add support for new runbook retention "Strategy" property #387
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| package runbooks | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
|
|
||
| "github.com/OctopusDeploy/go-octopusdeploy/v2/internal" | ||
| ) | ||
|
|
||
| type RunbookRetentionPolicy struct { | ||
| Strategy string `json:"Strategy"` | ||
| QuantityToKeep int32 `json:"QuantityToKeep"` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is QTK better as an "omitempty"?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ooooh this makes sense! very clever |
||
| Unit string `json:"Unit,omitempty"` | ||
| } | ||
|
|
||
| const ( | ||
| RunbookRetentionStrategyDefault string = "Default" | ||
| RunbookRetentionStrategyForever string = "Forever" | ||
| RunbookRetentionStrategyCount string = "Count" | ||
| ) | ||
|
|
||
| const ( | ||
| RunbookRetentionUnitDays string = "Days" | ||
| RunbookRetentionUnitItems string = "Items" | ||
| ) | ||
|
|
||
| func NewDefaultRunbookRetentionPolicy() *RunbookRetentionPolicy { | ||
| return &RunbookRetentionPolicy{ | ||
| Strategy: RunbookRetentionStrategyDefault, | ||
| QuantityToKeep: 100, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we are setting to a default strategy do we still need to set the QTK and unit?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned above, keeping for backwards compatibility.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ahh! |
||
| Unit: RunbookRetentionUnitItems, | ||
| } | ||
| } | ||
|
|
||
| func NewCountBasedRunbookRetentionPolicy(quantityToKeep int32, unit string) (*RunbookRetentionPolicy, error) { | ||
| if quantityToKeep < 1 { | ||
| return nil, internal.CreateInvalidParameterError("NewCountBasedRunbookRetentionPolicy", "quantityToKeep") | ||
| } | ||
|
|
||
| if unit != RunbookRetentionUnitDays && unit != RunbookRetentionUnitItems { | ||
| return nil, internal.CreateInvalidParameterError("NewCountBasedRunbookRetentionPolicy", "unit") | ||
| } | ||
|
|
||
| return &RunbookRetentionPolicy{ | ||
| Strategy: RunbookRetentionStrategyCount, | ||
| QuantityToKeep: quantityToKeep, | ||
| Unit: unit, | ||
| }, nil | ||
| } | ||
|
|
||
| func NewKeepForeverRunbookRetentionPolicy() *RunbookRetentionPolicy { | ||
| return &RunbookRetentionPolicy{ | ||
| Strategy: RunbookRetentionStrategyForever, | ||
| QuantityToKeep: 0, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned above, keeping for backwards compatibility. |
||
| Unit: RunbookRetentionUnitItems, | ||
| } | ||
| } | ||
|
|
||
| // MarshalJSON to handle backward compatibility with older server versions | ||
| func (r *RunbookRetentionPolicy) MarshalJSON() ([]byte, error) { | ||
| var fields struct { | ||
| QuantityToKeep int32 `json:"QuantityToKeep"` | ||
| ShouldKeepForever bool `json:"ShouldKeepForever"` | ||
| Unit string `json:"Unit"` | ||
| Strategy string `json:"Strategy"` | ||
| } | ||
|
|
||
| fields.QuantityToKeep = r.QuantityToKeep | ||
| fields.Unit = r.Unit | ||
| fields.Strategy = r.Strategy | ||
| fields.ShouldKeepForever = r.Strategy == RunbookRetentionStrategyForever | ||
|
|
||
| return json.Marshal(fields) | ||
| } | ||
|
|
||
| // MarshalJSON to handle backward compatibility with older server versions | ||
| func (r *RunbookRetentionPolicy) UnmarshalJSON(data []byte) error { | ||
| var fields struct { | ||
| QuantityToKeep int32 `json:"QuantityToKeep"` | ||
| ShouldKeepForever bool `json:"ShouldKeepForever"` | ||
| Unit string `json:"Unit"` | ||
| Strategy string `json:"Strategy"` | ||
| } | ||
|
|
||
| if err := json.Unmarshal(data, &fields); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| r.QuantityToKeep = fields.QuantityToKeep | ||
| r.Unit = fields.Unit | ||
|
|
||
| // If the Strategy field is present, use it directly | ||
| if fields.Strategy != "" { | ||
| r.Strategy = fields.Strategy | ||
| return nil | ||
| } | ||
|
|
||
| // Infer the Strategy based on other fields for backward compatibility | ||
| if fields.QuantityToKeep == 0 || fields.ShouldKeepForever == true { | ||
| r.Strategy = RunbookRetentionStrategyForever | ||
| return nil | ||
| } | ||
|
|
||
| if fields.QuantityToKeep == 100 && r.Unit == RunbookRetentionUnitItems { | ||
| r.Strategy = RunbookRetentionStrategyDefault | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| return nil | ||
| } | ||
|
|
||
| r.Strategy = RunbookRetentionStrategyCount | ||
| return nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| package runbooks | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "testing" | ||
|
|
||
| "github.com/kinbiko/jsonassert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestCountBasedRunbookRetentionPolicyMarshalJSON(t *testing.T) { | ||
| expectedJson := `{ | ||
| "Strategy": "Count", | ||
| "QuantityToKeep": 10, | ||
| "ShouldKeepForever": false, | ||
| "Unit": "Days" | ||
| }` | ||
|
|
||
| runbookRetentionPolicy, err := NewCountBasedRunbookRetentionPolicy(10, RunbookRetentionUnitDays) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, runbookRetentionPolicy) | ||
|
|
||
| runbookRetentionPolicyAsJSON, err := json.Marshal(runbookRetentionPolicy) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, runbookRetentionPolicyAsJSON) | ||
|
|
||
| jsonassert.New(t).Assertf(expectedJson, string(runbookRetentionPolicyAsJSON)) | ||
| } | ||
|
|
||
| func TestKeepForeverRunbookRetentionPolicyMarshalJSON(t *testing.T) { | ||
| expectedJson := `{ | ||
| "Strategy": "Forever", | ||
| "QuantityToKeep": 0, | ||
| "ShouldKeepForever": true, | ||
| "Unit": "Items" | ||
| }` | ||
|
|
||
| runbookRetentionPolicy := NewKeepForeverRunbookRetentionPolicy() | ||
| require.NotNil(t, runbookRetentionPolicy) | ||
| runbookRetentionPolicyAsJSON, err := json.Marshal(runbookRetentionPolicy) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, runbookRetentionPolicyAsJSON) | ||
|
|
||
| jsonassert.New(t).Assertf(expectedJson, string(runbookRetentionPolicyAsJSON)) | ||
| } | ||
|
|
||
| func TestDefaultRunbookRetentionPolicyMarshalJSON(t *testing.T) { | ||
| expectedJson := `{ | ||
| "Strategy": "Default", | ||
| "QuantityToKeep": 100, | ||
| "ShouldKeepForever": false, | ||
| "Unit": "Items" | ||
| }` | ||
|
|
||
| runbookRetentionPolicy := NewDefaultRunbookRetentionPolicy() | ||
| require.NotNil(t, runbookRetentionPolicy) | ||
| runbookRetentionPolicyAsJSON, err := json.Marshal(runbookRetentionPolicy) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, runbookRetentionPolicyAsJSON) | ||
|
|
||
| jsonassert.New(t).Assertf(expectedJson, string(runbookRetentionPolicyAsJSON)) | ||
| } | ||
|
|
||
| func TestCountBasedRunbookRetentionPolicyWithStrategyUnmarshalJSON(t *testing.T) { | ||
| inputJSON := `{ | ||
| "Strategy": "Count", | ||
| "QuantityToKeep": 10, | ||
| "Unit": "Days" | ||
| }` | ||
|
|
||
| var runbookRetentionPolicy RunbookRetentionPolicy | ||
| err := json.Unmarshal([]byte(inputJSON), &runbookRetentionPolicy) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, runbookRetentionPolicy) | ||
| require.Equal(t, RunbookRetentionStrategyCount, runbookRetentionPolicy.Strategy) | ||
| require.Equal(t, int32(10), runbookRetentionPolicy.QuantityToKeep) | ||
| require.Equal(t, RunbookRetentionUnitDays, runbookRetentionPolicy.Unit) | ||
| } | ||
|
|
||
| func TestKeepForeverRunbookRetentionPolicyWithStrategyUnmarshalJSON(t *testing.T) { | ||
| inputJSON := `{ | ||
| "Strategy": "Forever", | ||
| "QuantityToKeep": 0, | ||
| "Unit": "Items", | ||
| "ShouldKeepForever": true | ||
| }` | ||
|
|
||
| var runbookRetentionPolicy RunbookRetentionPolicy | ||
| err := json.Unmarshal([]byte(inputJSON), &runbookRetentionPolicy) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, runbookRetentionPolicy) | ||
| require.Equal(t, RunbookRetentionStrategyForever, runbookRetentionPolicy.Strategy) | ||
| require.Equal(t, int32(0), runbookRetentionPolicy.QuantityToKeep) | ||
| require.Equal(t, RunbookRetentionUnitItems, runbookRetentionPolicy.Unit) | ||
| } | ||
|
|
||
| func TestDefaultRunbookRetentionPolicyWithStrategyUnmarshalJSON(t *testing.T) { | ||
| inputJSON := `{ | ||
| "Strategy": "Default", | ||
| "QuantityToKeep": 100, | ||
| "Unit": "Items", | ||
| "ShouldKeepForever": false | ||
| }` | ||
|
|
||
| var runbookRetentionPolicy RunbookRetentionPolicy | ||
| err := json.Unmarshal([]byte(inputJSON), &runbookRetentionPolicy) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, runbookRetentionPolicy) | ||
| require.Equal(t, RunbookRetentionStrategyDefault, runbookRetentionPolicy.Strategy) | ||
| require.Equal(t, int32(100), runbookRetentionPolicy.QuantityToKeep) | ||
| require.Equal(t, RunbookRetentionUnitItems, runbookRetentionPolicy.Unit) | ||
| } | ||
|
|
||
| func TestCountBasedRunbookRetentionPolicyWithoutStrategyUnmarshalJSON(t *testing.T) { | ||
| inputJSON := `{ | ||
| "QuantityToKeep": 10, | ||
| "Unit": "Days", | ||
| "ShouldKeepForever": false | ||
| }` | ||
|
|
||
| var runbookRetentionPolicy RunbookRetentionPolicy | ||
| err := json.Unmarshal([]byte(inputJSON), &runbookRetentionPolicy) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, runbookRetentionPolicy) | ||
| require.Equal(t, RunbookRetentionStrategyCount, runbookRetentionPolicy.Strategy) | ||
| require.Equal(t, int32(10), runbookRetentionPolicy.QuantityToKeep) | ||
| require.Equal(t, RunbookRetentionUnitDays, runbookRetentionPolicy.Unit) | ||
| } | ||
|
|
||
| func TestKeepForeverRunbookRetentionPolicyWithoutStrategyUnmarshalJSON(t *testing.T) { | ||
| inputJSON := `{ | ||
| "QuantityToKeep": 0, | ||
| "Unit": "Items", | ||
| "ShouldKeepForever": true | ||
| }` | ||
|
|
||
| var runbookRetentionPolicy RunbookRetentionPolicy | ||
| err := json.Unmarshal([]byte(inputJSON), &runbookRetentionPolicy) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, runbookRetentionPolicy) | ||
| require.Equal(t, RunbookRetentionStrategyForever, runbookRetentionPolicy.Strategy) | ||
| require.Equal(t, int32(0), runbookRetentionPolicy.QuantityToKeep) | ||
| require.Equal(t, RunbookRetentionUnitItems, runbookRetentionPolicy.Unit) | ||
| } | ||
|
|
||
| func TestDefaultRunbookRetentionPolicyWithoutStrategyUnmarshalJSON(t *testing.T) { | ||
| inputJSON := `{ | ||
| "QuantityToKeep": 100, | ||
| "Unit": "Items", | ||
| "ShouldKeepForever": false | ||
| }` | ||
|
|
||
| var runbookRetentionPolicy RunbookRetentionPolicy | ||
| err := json.Unmarshal([]byte(inputJSON), &runbookRetentionPolicy) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, runbookRetentionPolicy) | ||
| require.Equal(t, RunbookRetentionStrategyDefault, runbookRetentionPolicy.Strategy) | ||
| require.Equal(t, int32(100), runbookRetentionPolicy.QuantityToKeep) | ||
| require.Equal(t, RunbookRetentionUnitItems, runbookRetentionPolicy.Unit) | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
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 we be including
omitemptyfor strategy? I'd thought it was a required field. Is it because of backwards compatibility?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.
You mean line 64 and line 81 in pkg/runbooks/runbook_retention_policy.go?
Yeah that is a mistake