Fix defaults for autopilot config update #10559
Conversation
jkirschner-hashicorp
added
type/bug
agent/operator_endpoint_test.go
Outdated
| if reply.LastContactThreshold != 200 * time.Millisecond || | ||
| reply.MaxTrailingLogs != 250 || | ||
| reply.ServerStabilizationTime != 10 * time.Second { | ||
| t.Fatalf("optional parameters not set to defaults: %#v", reply) | ||
| } |
There was a problem hiding this comment.
This particular test uses the stdlib style of checking values but most of our new tests use testify/require. The linter is complaining about the formatting here, but I think instead of simply fixing the formatting using the following would make for a stricter test:
expected := structs.AutopilotConfig{
...
}
require.Equal(t, expected, reply)There was a problem hiding this comment.
Agreed that comparing all fields would be a much better test
| @@ -0,0 +1,3 @@ | |||
| ```release-note:bug | |||
| api: Fix default values used for optional fields in autopilot configuration update (POST to `/v1/operator/autopilot/configuration`) [[GH-10558](https://github.com/hashicorp/consul/issues/10558)] | |||
There was a problem hiding this comment.
the changelog tool will add the github link for us. I guess it's not strictly necessary to remove it, just an FYI that you don't need to add it
| api: Fix default values used for optional fields in autopilot configuration update (POST to `/v1/operator/autopilot/configuration`) [[GH-10558](https://github.com/hashicorp/consul/issues/10558)] | |
| api: Fix default values used for optional fields in autopilot configuration update (POST to `/v1/operator/autopilot/configuration`) | |
api/operator_autopilot.go
Outdated
| func NewAutopilotConfiguration() AutopilotConfiguration { | ||
| // Defines default values for this type, consistent with | ||
| // https://www.consul.io/api-docs/operator/autopilot#parameters-1 |
There was a problem hiding this comment.
Generally it is preferred to use godoc comments instead of inline comments. Godoc comments will end up in the generated docs at https://pkg.go.dev/github.com/hashicorp/consul/api. More details here: https://blog.golang.org/godoc
| func NewAutopilotConfiguration() AutopilotConfiguration { | |
| // Defines default values for this type, consistent with | |
| // https://www.consul.io/api-docs/operator/autopilot#parameters-1 | |
| // NewAutopilotConfiguration returns a AutopilotConfiguration with all fields set to the default values. | |
| // See https://www.consul.io/api-docs/operator/autopilot#parameters-1 | |
| func NewAutopilotConfiguration() AutopilotConfiguration { | |
|
Will address these comments later today - thanks! |
Previously, for a POST request to the /v1/operator/autopilot/configuration endpoint, any fields not included in the payload were set to a zero-initialized value rather than the documented default value. Now, if an optional field is not included in the payload, it will be set to its documented default value: - CleanupDeadServers: true - LastContactThreshold: "200ms" - MaxTrailingLogs: 250 - MinQuorum: 0 - ServerStabilizationTime: "10s" - RedundancyZoneTag: "" - DisableUpgradeMigration: false - UpgradeVersionTag: ""
jkirschner-hashicorp
force-pushed
the
fix-autopilot-config-post-default-values
branch
from
8468d4b
to
1353ee4
Compare
| @@ -58,6 +58,23 @@ type AutopilotConfiguration struct { | |||
| ModifyIndex uint64 | |||
| } | |||
|
|
|||
| // Defines default values for the AutopilotConfiguration type, consistent with | |||
There was a problem hiding this comment.
Moved function description above declaration, per review
| body := bytes.NewBuffer([]byte(`{"CleanupDeadServers": false}`)) | ||
| expected := structs.AutopilotConfig{ |
There was a problem hiding this comment.
Improved test by forcing comparison of full configuration object (except CreateIndex and ModifyIndex), per review
There was a problem hiding this comment.
Side note: I got a bit tripped up between AutopilotConfig and AutopilotConfiguration
Closes #10558
Previously, for a POST request to the /v1/operator/autopilot/configuration
endpoint, any fields not included in the payload were set to a zero-initialized
value rather than the documented default value.
Now, if an optional field is not included in the payload, it will be set to its
documented default value: