-
Notifications
You must be signed in to change notification settings - Fork 6
PMM-8459 Ability to disable updates. #228
base: master
Are you sure you want to change the base?
Conversation
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.
LGTM, but let's change order.
| res, err := serverClient.Default.Server.ChangeSettings(&server.ChangeSettingsParams{ | ||
| Body: server.ChangeSettingsBody{ | ||
| EnableUpdates: true, | ||
| }, | ||
| Context: pmmapitests.Context, | ||
| }) | ||
| require.NoError(t, err) | ||
| assert.False(t, res.Payload.Settings.UpdatesDisabled) | ||
| assert.Empty(t, err) | ||
|
|
||
| resg, err := serverClient.Default.Server.GetSettings(nil) | ||
| require.NoError(t, err) | ||
| assert.False(t, resg.Payload.Settings.UpdatesDisabled) |
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.
let's first disable updates and then enable updates.
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.
Fixed.
server/settings_test.go
Outdated
|
|
||
| res, err = serverClient.Default.Server.ChangeSettings(&server.ChangeSettingsParams{ | ||
| Body: server.ChangeSettingsBody{ | ||
| DisableUpdates: false, |
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.
it does nothing actually.
Can we check that DisableUpdates: true disables updates?
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.
Fixed.
| Context: pmmapitests.Context, | ||
| }) | ||
| require.NoError(t, err) | ||
| assert.False(t, res.Payload.Settings.UpdatesDisabled) |
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.
shouldn't it be True?
No description provided.