Skip to content
This repository was archived by the owner on Aug 19, 2021. It is now read-only.

PMM-8459 Ability to disable updates.#228

Open
JiriCtvrtka wants to merge 6 commits into
masterfrom
PMM-8459-disable-updates
Open

PMM-8459 Ability to disable updates.#228
JiriCtvrtka wants to merge 6 commits into
masterfrom
PMM-8459-disable-updates

Conversation

@JiriCtvrtka

Copy link
Copy Markdown
Contributor

No description provided.

@JiriCtvrtka JiriCtvrtka marked this pull request as ready for review July 29, 2021 07:57

@BupycHuk BupycHuk left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread server/settings_test.go
Comment on lines +58 to +70
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)

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Comment thread server/settings_test.go Outdated

res, err = serverClient.Default.Server.ChangeSettings(&server.ChangeSettingsParams{
Body: server.ChangeSettingsBody{
DisableUpdates: false,

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@JiriCtvrtka JiriCtvrtka requested a review from BupycHuk August 5, 2021 07:00
Comment thread server/settings_test.go
Context: pmmapitests.Context,
})
require.NoError(t, err)
assert.False(t, res.Payload.Settings.UpdatesDisabled)

Copy link
Copy Markdown
Contributor

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?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants