-
Notifications
You must be signed in to change notification settings - Fork 177
PMM-14253 Update flow #4525
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
base: PMM-13652-native-navigation
Are you sure you want to change the base?
PMM-14253 Update flow #4525
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## PMM-13652-native-navigation #4525 +/- ##
===============================================================
- Coverage 44.98% 44.98% -0.01%
===============================================================
Files 356 356
Lines 44808 44813 +5
===============================================================
+ Hits 20157 20158 +1
- Misses 22999 23002 +3
- Partials 1652 1653 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hello, would like some feedback on the BE part of this. I've added a separate snooze updates api endpoint which handled the logic related to snoozing:
Also I've extended settings with snooze duration (currently 7 days) - time after snooze for which an update modal is not shown to the user (configurable via env variable - maybe just for QAs?) What's missing:
EDIT: |
api/server/v1/server.proto
Outdated
description: "Returns PMM Server update status." | ||
}; | ||
} | ||
// SnoozeUpdate snoozes updates for currently logged in user and returns the count and timestamp |
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.
// SnoozeUpdate snoozes updates for currently logged in user and returns the count and timestamp | |
// SnoozeUpdate snoozes updates for an authenticated user and returns the count and timestamp |
api/server/v1/server.proto
Outdated
optional bool enable_backup_management = 12; | ||
// Enable Access Control | ||
optional bool enable_access_control = 13; | ||
// A number of full days for which an update is snoozed. Should have a suffix in JSON: 2592000s, 43200m, 720h. |
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.
// A number of full days for which an update is snoozed. Should have a suffix in JSON: 2592000s, 43200m, 720h. | |
// A number of full days for which an update is snoozed, i.e. a multiple of 24h: 2592000s, 43200m, 720h. |
I wonder if it wouldn't be easier to provide an uint
here? That'd be simple to read and test, no?
api/user/v1/user.proto
Outdated
string snoozed_pmm_version = 4; | ||
reserved 5; | ||
google.protobuf.Timestamp snoozed_at = 6; | ||
uint32 snoozed_count = 7; |
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.
uint32 snoozed_count = 7; | |
uint32 snooze_count = 7; |
}, | ||
112: { | ||
`UPDATE settings | ||
SET settings = settings || '{"updates": {"snooze_duration": 604800000000000}}' |
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 provide a comment here, i.e. how many days it is.
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.
Is it 7?
managed/services/server/server.go
Outdated
}, nil | ||
} | ||
|
||
// Snooze updates for specific pmm version for current user |
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.
// Snooze updates for specific pmm version for current user | |
// SnoozeUpdate snoozes the updates for specific pmm version for current user. |
managed/services/user/user.go
Outdated
return nil, err | ||
} | ||
|
||
var snoozedAt *timestamppb.Timestamp |
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.
If you put this block past line 85, there will be no need to define the variable.
managed/services/user/user.go
Outdated
if req.SnoozedPmmVersion != nil { | ||
|
||
// Keep for backwards compatibility, prefer the new snooze updates:endpoint | ||
if req.SnoozedPmmVersion != nil && req.SnoozedPmmVersion != &userInfo.SnoozedPMMVersion { |
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.
I think we should check the values here, not the pointers, since pointers will be different.
managed/services/user/user.go
Outdated
return nil, e | ||
} | ||
|
||
var snoozedAt *timestamppb.Timestamp |
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.
Same - let's put this block after line 143.
managed/utils/envvars/parser.go
Outdated
continue | ||
} | ||
envSettings.EnableUpdates = &b | ||
case "PMM_UPDATES_SNOOZE_DURATION": |
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.
case "PMM_UPDATES_SNOOZE_DURATION": | |
case "PMM_UPDATE_SNOOZE_DURATION": |
managed/utils/envvars/parser.go
Outdated
} | ||
envSettings.EnableUpdates = &b | ||
case "PMM_UPDATES_SNOOZE_DURATION": | ||
if envSettings.UpdatesSnoozeDuration, err = parseStringDuration(v); err != nil { |
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.
if envSettings.UpdatesSnoozeDuration, err = parseStringDuration(v); err != nil { | |
if envSettings.UpdateSnoozeDuration, err = parseStringDuration(v); err != nil { |
api/user/v1/user.proto
Outdated
string snoozed_pmm_version = 4; | ||
reserved 5; | ||
google.protobuf.Timestamp snoozed_at = 6; | ||
uint32 snoozed_count = 7; |
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.
uint32 snoozed_count = 7; | |
uint32 snooze_count = 7; |
return resp, nil | ||
} | ||
|
||
// SnoozeUpdate snoozes the updates for specific pmm version for current user |
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.
🚫 [golangci] reported by reviewdog 🐶
Comment should end in a period (godot)
PMM-14253
Link to the Feature Build: SUBMODULES-4038
If this PR adds or removes or alters one or more API endpoints, please review and add or update the relevant API documents as well:
If this PR is related to some other PRs in this or other repositories, please provide links to those PRs: