-
Notifications
You must be signed in to change notification settings - Fork 898
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
[Workspace] Consume workspace id in saved object client #6014
[Workspace] Consume workspace id in saved object client #6014
Conversation
86b2934
to
23f5907
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6014 +/- ##
==========================================
- Coverage 67.12% 67.08% -0.04%
==========================================
Files 3323 3324 +1
Lines 64291 64309 +18
Branches 10336 10341 +5
==========================================
- Hits 43153 43144 -9
- Misses 18615 18686 +71
+ Partials 2523 2479 -44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
d801828
to
645d605
Compare
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.
left some comments
src/plugins/saved_objects_management/public/lib/resolve_import_errors.test.ts
Outdated
Show resolved
Hide resolved
...ins/saved_objects_management/public/management_section/objects_table/saved_objects_table.tsx
Outdated
Show resolved
Hide resolved
...ins/saved_objects_management/public/management_section/objects_table/saved_objects_table.tsx
Outdated
Show resolved
Hide resolved
...ins/saved_objects_management/public/management_section/objects_table/saved_objects_table.tsx
Outdated
Show resolved
Hide resolved
@@ -227,6 +234,11 @@ export class SavedObjectsClient { | |||
this.batchQueue = []; | |||
} | |||
|
|||
public setCurrentWorkspace(workspaceId: string): boolean { |
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.
why do we need to have a separate set workspace id when using saved object client? Can workspaces field be within the options? If each workspace should have one saved object client instance, we can add workspace id in the constructor
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.
This method is used in workspace.start lifeCycle, which indicates current workspace id. We support workspaces field within options, but it is import that we add a internal currentWorkspaceId
because we want to make workspaces
totally transparent to other plugins so that no matter which plugin calls savedObjectsClient methods, the client will append workspaces fields when doing the real request automatically.
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.
What do you mean to make the workspaces transparent? this is to set the workspace id not get, right? plus, why would plugins need to get current workspace id from the saved objects client. Can we have the workspace id in the global context instead of setting it specific for saved object client, is it the only place where it needs to know the current workspace?
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.
The setCurrentWorkspace
will only be called by workspace plugin when setup, and once the currentWorkspaceId
is set, the workspaces field will be automatically append into the final request payload accordingly.
if (options.hasOwnProperty('workspaces')) { | ||
finalWorkspaces = options.workspaces; | ||
} else if (typeof currentWorkspaceId === 'string') { | ||
finalWorkspaces = [currentWorkspaceId]; |
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.
what's the difference of the currentworkspace id vs the one from the payload/options?
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.
The workspaces fields inside payload/options has a higher priority than currentWorkspaceId. Most of the cases there will not be workspaces fields in options, except some plugins want to overwrite the workspaces explicitly.
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.
Can't we just set workspace id into options when using the saved object client?
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.
There are 2 sources of workspaces:
- workspaces inside options.
- currentWorkspaceId in the instance.
the workspaces inside options
has higher priority than the current workspaceId, if somewhere calls a method by specifying workspaces inside options
, we will honor that and use exactly what they pass to the request payload.
We add currentWorkspaceId in the instance
because actually there is few case that plugins want to overwrite the workspaces inside options
, by using the currentWorkspaceId in the instance
, dashboards page / visualizations page / data source management page will support workspace automatically without a single line of code changes because currentWorkspaceId is already set by workspace plugin when they call any CRUD operations through saved objects client.
@@ -131,6 +131,7 @@ export async function resolveSavedObjectsImportErrors({ | |||
retries, | |||
createNewCopies, | |||
dataSourceId, | |||
workspaces, | |||
}; | |||
const checkConflictsResult = await checkConflicts(checkConflictsParams); |
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.
how would workspaces checked in checkConflicts?
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.
public start(core: CoreStart) { | ||
this.coreStart = core; | ||
|
||
this.currentWorkspaceSubscription = this._changeSavedObjectCurrentWorkspace(); |
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.
how is this currentWorkspaceSubscription used? should it be observable instead?
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.
The currentWorkspaceSubscription
is used for stopping the subscription in stop
lifeCycle.
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.
okay, how is this observable currentWorkspaceId$ initialized?
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.
The currentWorkspaceId$
will be initialized by a workspace client. You can find the usage here: #6094
645d605
to
811eb0c
Compare
@@ -227,6 +234,11 @@ export class SavedObjectsClient { | |||
this.batchQueue = []; | |||
} | |||
|
|||
public setCurrentWorkspace(workspaceId: string): boolean { |
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.
What do you mean to make the workspaces transparent? this is to set the workspace id not get, right? plus, why would plugins need to get current workspace id from the saved objects client. Can we have the workspace id in the global context instead of setting it specific for saved object client, is it the only place where it needs to know the current workspace?
public start(core: CoreStart) { | ||
this.coreStart = core; | ||
|
||
this.currentWorkspaceSubscription = this._changeSavedObjectCurrentWorkspace(); |
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.
okay, how is this observable currentWorkspaceId$ initialized?
if (options.hasOwnProperty('workspaces')) { | ||
finalWorkspaces = options.workspaces; | ||
} else if (typeof currentWorkspaceId === 'string') { | ||
finalWorkspaces = [currentWorkspaceId]; |
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.
Can't we just set workspace id into options when using the saved object client?
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
75f5530
to
c553a36
Compare
…roject#6014) * feat: consume current workspace in saved objects management and saved objects client Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add unit test for each change Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: update snapshot of unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update snapshot Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * revert: saved object management changes Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add CHANGELOG Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: address some comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: address comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove useless return Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: bootstrap error Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update CHANGELOG Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update CHANGELOG Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
…roject#6014) * feat: consume current workspace in saved objects management and saved objects client Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add unit test for each change Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: update snapshot of unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update snapshot Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * revert: saved object management changes Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add CHANGELOG Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: address some comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: address comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove useless return Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: bootstrap error Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update CHANGELOG Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update CHANGELOG Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
…roject#6014) (#290) * feat: consume current workspace in saved objects management and saved objects client * feat: add unit test * feat: add unit test for each change * fix: update snapshot of unit test * fix: unit test * fix: unit test * fix: unit test * feat: update snapshot * revert: saved object management changes * feat: add CHANGELOG * feat: address some comment * feat: address comment * feat: remove useless return * fix: bootstrap error * feat: update CHANGELOG * feat: update CHANGELOG * feat: optimize code * feat: update comment --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
* feat: consume current workspace in saved objects management and saved objects client Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add unit test for each change Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: update snapshot of unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update snapshot Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * revert: saved object management changes Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add CHANGELOG Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: address some comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: address comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove useless return Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: bootstrap error Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update CHANGELOG Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update CHANGELOG Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com> (cherry picked from commit 8f0885d) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
* feat: consume current workspace in saved objects management and saved objects client Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add unit test for each change Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: update snapshot of unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update snapshot Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * revert: saved object management changes Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add CHANGELOG Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: address some comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: address comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove useless return Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: bootstrap error Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update CHANGELOG Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update CHANGELOG Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com> (cherry picked from commit 8f0885d) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
This PR adds a new private property in saved objects client to make the client be aware of current workspace. Furthermore when user tries to do Create/Update/Find operations on saved objects, the property will take effect if set and append itself to request payload to indicate it is doing the operation under the specific workspace.
Issues Resolved
closes #6027
Screenshot
Testing the changes
Check List
yarn test:jest
yarn test:jest_integration