PMM-8459 Ability to disable updates.#808
Conversation
Codecov Report
@@ Coverage Diff @@
## PMM-2.0 #808 +/- ##
===========================================
+ Coverage 49.48% 49.50% +0.01%
===========================================
Files 167 167
Lines 18808 18818 +10
===========================================
+ Hits 9308 9316 +8
- Misses 8484 8485 +1
- Partials 1016 1017 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Updates can be disabled only when telemetry is disabled too. |
BupycHuk
left a comment
BupycHuk
left a comment
There was a problem hiding this comment.
Do not forget about API tests.
services/server/server_test.go
Outdated
|
|
||
| s.envSettings.DisableTelemetry = true | ||
| s.envSettings.EnableUpdates = true | ||
| expected = status.New(codes.FailedPrecondition, "Updates are enabled via ENABLE_UPDATES environment variable.") |
There was a problem hiding this comment.
Do we have ENABLE_UPDATES env variable? As far as I know we have only DISABLE_UPDATES
There was a problem hiding this comment.
let's rewrite test taking into account that we can't enable updates if it's disabled.
services/server/server.go
Outdated
| if req.DisableUpdates && s.envSettings.EnableTelemetry { | ||
| return status.Error(codes.FailedPrecondition, "Updates cannot be disabled because telemetry is enabled via ENABLE_TELEMETRY environment variable.") | ||
| } |
There was a problem hiding this comment.
updates and telemetry is independent, so we can remove this check.
There was a problem hiding this comment.
Here https://perconacorp.slack.com/archives/C03J8FZFU/p1626470324102700 is discussion that they are coupled. Ok I will change everything related to it.
There was a problem hiding this comment.
Please discuss it with Roma, Steve or Denys. But previously users were able to disable updates using env variable without disabling telemetry.
There was a problem hiding this comment.
that was just assumption from Steve (that disable doesn't work because of telemetry). It is not a requirement.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
services/server/server.go
Outdated
| if req.DisableUpdates && !s.envSettings.DisableUpdates { | ||
| return status.Error(codes.FailedPrecondition, "Updates are enabled via DISABLE_UPDATES environment variable.") | ||
| } |
There was a problem hiding this comment.
let's remove it. s.envSettings.DisableUpdates is false by default, so users won't be able to disable updates.
percona-csalguero
left a comment
percona-csalguero
left a comment
There was a problem hiding this comment.
LGTM, just address the changes requested by Nurlan.
|
@BupycHuk Take a look if I got you right. API tests added in description. |
5 participants
PMM-8459
Build: PMM-8509 Check for updates Percona-Lab/pmm-submodules#1939
Links to other linked pull requests (optional).
pmm: PMM-8459 Ability to disable updates. pmm#771
api-tests: PMM-8459 Ability to disable updates. Percona-Lab/pmm-api-tests#228