Update settings api to use ResourceContext over ModificationInfo
#1633
Conversation
…ethods, instead of `ModificationInfo` directly, to allow for a fallback implementation between `ResourceContext` and `ModificationInfo`
|
| @@ -60,6 +117,13 @@ type SettingsModificationInfo struct { | |||
| NonModifiablePaths []interface{} `json:"nonModifiablePaths"` | |||
| } | |||
|
|
|||
| type SettingsResourceContext struct { | |||
| Operations []string `json:"operations"` | |||
| Movable *bool `json:"modifications:movable"` | |||
There was a problem hiding this comment.
To the reviewer. I chose a pointer over its own struct type, the problem here is that the param Movable is optional, and logic code in method isMovable() depends on it.
If preferred, I could change it to a NullBool type and implement the unmarshaling myself. Since is not done anywhere else in the code i chose *bool over NullBool.
There was a problem hiding this comment.
By now, I wasn't aware of the NullBool type in GO. But as I see it, it is part of the SQL library and for that reason wouldn't it even consider. For me pointer is just enough
E2E Test Results 4 files - 1 268 suites - 133 21m 55s ⏱️ - 52m 36s Results for commit 7442a1e. ± Comparison against base commit 21c774b. This pull request removes 2 and adds 3 tests. Note that renamed tests count towards both. |
| logger.Info("Deleting %d objects of type %q...", len(settingsObjects), s) | ||
| for _, settingsObject := range settingsObjects { | ||
| if !settingsObject.IsDeletable() { | ||
| //@Reviewer should here also be a log as it is in the Delete() method |
There was a problem hiding this comment.
For consistency, it can be added. But if you doesn't see the purpose, I wouldn't
| // skip discarded settings objects | ||
| if shouldDiscard, reason := filters.Get(o.SchemaId).ShouldDiscard(contentUnmarshalled); shouldFilterSettings() && shouldDiscard { | ||
| log.WithFields(field.Type(o.SchemaId), field.F("object", o)).Debug("Discarded setting object %q (%s). Reason: %s", o.ObjectId, o.SchemaId, reason) | ||
| // skip discarded settings settingsObjects |
| WriteOperation = "write" | ||
| //ReadOperation = "read" | ||
| ) | ||
|
|
||
| // DownloadSettingsObject is the response type for the ListSettings operation | ||
| type DownloadSettingsObject struct { |
There was a problem hiding this comment.
Why not move to a separate file
| @@ -60,6 +117,13 @@ type SettingsModificationInfo struct { | |||
| NonModifiablePaths []interface{} `json:"nonModifiablePaths"` | |||
| } | |||
|
|
|||
| type SettingsResourceContext struct { | |||
| Operations []string `json:"operations"` | |||
| Movable *bool `json:"modifications:movable"` | |||
There was a problem hiding this comment.
By now, I wasn't aware of the NullBool type in GO. But as I see it, it is part of the SQL library and for that reason wouldn't it even consider. For me pointer is just enough
What this PR does / Why we need it:
Update settings API to use
ResourceContextoverModificationInfo,ModificationInfois deprecated by the API and only used as a fallbackSpecial notes for your reviewer:
Does this PR introduce a user-facing change?