Skip to content
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]Optional workspaces params in repository #5162

Conversation

SuZhou-Joe
Copy link
Member

@SuZhou-Joe SuZhou-Joe commented Sep 30, 2023

Description

As workspace will be a new concept to organize saved objects, this PR add a new field workspaces to indicate which workspaces the objects belong to and consume the field in related methods.

  • Add workspaces field in saved objects mappings
  • Consume worksapces when create object
  • Consume workspaces when bulkCreate objects
  • Consume workspaces when checkConflicts
  • Consume workspaces when validating reference

Issues Resolved

Partial #4944

Screenshot

N/A

Testing the changes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@SuZhou-Joe
Copy link
Member Author

SuZhou-Joe commented Sep 30, 2023

#5163

gaobinlong and others added 13 commits September 30, 2023 10:51
Signed-off-by: SuZhou-Joe <[email protected]>
…oject#189)

* feat: optimize logic when checkConflict and bulkCreate

Signed-off-by: SuZhou-Joe <[email protected]>

* feat: add options.workspace check

Signed-off-by: SuZhou-Joe <[email protected]>

* feat: throw error when workspace check error in repository create

Signed-off-by: SuZhou-Joe <[email protected]>

* feat: modify judgement

Signed-off-by: SuZhou-Joe <[email protected]>

* feat: always get objects from DB when create-with-override

Signed-off-by: SuZhou-Joe <[email protected]>

---------

Signed-off-by: SuZhou-Joe <[email protected]>
Signed-off-by: SuZhou-Joe <[email protected]>
Signed-off-by: SuZhou-Joe <[email protected]>
Signed-off-by: SuZhou-Joe <[email protected]>
Signed-off-by: SuZhou-Joe <[email protected]>
Signed-off-by: SuZhou-Joe <[email protected]>
Signed-off-by: SuZhou-Joe <[email protected]>
Signed-off-by: SuZhou-Joe <[email protected]>
Signed-off-by: SuZhou-Joe <[email protected]>
@SuZhou-Joe SuZhou-Joe force-pushed the feature/optional-workspaces-params-in-repository branch from bf93497 to f4b86f0 Compare September 30, 2023 02:51
@codecov
Copy link

codecov bot commented Sep 30, 2023

Codecov Report

Merging #5162 (3288961) into main (9e3e3a7) will increase coverage by 0.08%.
Report is 4 commits behind head on main.
The diff coverage is 69.62%.

@@            Coverage Diff             @@
##             main    #5162      +/-   ##
==========================================
+ Coverage   66.79%   66.87%   +0.08%     
==========================================
  Files        3286     2576     -710     
  Lines       63129    48473   -14656     
  Branches    10053     8706    -1347     
==========================================
- Hits        42164    32418    -9746     
+ Misses      18490    13928    -4562     
+ Partials     2475     2127     -348     
Flag Coverage Δ
Linux_1 ?
Linux_2 55.31% <69.62%> (+0.01%) ⬆️
Linux_3 43.74% <0.00%> (-0.09%) ⬇️
Linux_4 35.28% <0.00%> (-0.07%) ⬇️
Windows_1 ?
Windows_2 ?
Windows_3 43.74% <0.00%> (-0.10%) ⬇️
Windows_4 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
.../core/public/saved_objects/saved_objects_client.ts 92.55% <ø> (ø)
...ore/server/saved_objects/import/check_conflicts.ts 100.00% <ø> (ø)
...erver/saved_objects/import/create_saved_objects.ts 100.00% <ø> (ø)
...erver/saved_objects/import/import_saved_objects.ts 100.00% <100.00%> (ø)
...rver/saved_objects/import/resolve_import_errors.ts 96.42% <ø> (ø)
...server/saved_objects/import/validate_references.ts 94.23% <100.00%> (+0.75%) ⬆️
...d_objects/migrations/core/build_active_mappings.ts 86.66% <ø> (ø)
...e/server/saved_objects/serialization/serializer.ts 100.00% <100.00%> (ø)
...ved_objects/service/lib/search_dsl/query_params.ts 92.13% <100.00%> (+0.46%) ⬆️
...saved_objects/service/lib/search_dsl/search_dsl.ts 100.00% <ø> (ø)
... and 10 more

... and 806 files with indirect coverage changes

@kavilla kavilla linked an issue Oct 4, 2023 that may be closed by this pull request
@kavilla
Copy link
Member

kavilla commented Oct 4, 2023

Just a heads up @SuZhou-Joe, modified the PR description.

if it reads closes #issue number it will auto attach the issue and then the issue will be closed on merged. even if there is a word before closes.

Not a big deal, can always re-open the issue if it does close.

@SuZhou-Joe
Copy link
Member Author

Just a heads up @SuZhou-Joe, modified the PR description.

if it reads closes #issue number it will auto attach the issue and then the issue will be closed on merged. even if there is a word before closes.

Not a big deal, can always re-open the issue if it does close.

Thanks @kavilla , modified the PR description.

@@ -121,6 +127,7 @@ async function fetchObjectsToExport({
search,
perPage: exportSizeLimit,
namespaces: namespace ? [namespace] : undefined,
...(workspaces ? { workspaces } : {}),
Copy link
Member

Choose a reason for hiding this comment

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

are we passing an object instead of an array?

Copy link
Member Author

@SuZhou-Joe SuZhou-Joe Oct 17, 2023

Choose a reason for hiding this comment

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

Workspaces is an array, in here if workspaces is null, ...({}) will have no impact on the params that passed to find, if we use workspaces: workspaces ? workspaces : undefined, there will be an extra { ...., workspaces: undefined } in find params.

src/core/server/saved_objects/import/regenerate_ids.ts Outdated Show resolved Hide resolved
Copy link
Member

@zengyan-amazon zengyan-amazon left a comment

Choose a reason for hiding this comment

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

for all these workspace operations, can we do it with interceptors like savedObjectClientWrapper instead of making surgical changes the repository deeply?

@SuZhou-Joe
Copy link
Member Author

SuZhou-Joe commented Oct 17, 2023

for all these workspace operations, can we do it with interceptors like savedObjectClientWrapper instead of making surgical changes the repository deeply?

You mean the preflightCheck on conflict with workspace right? That's a good advice and I haven't think about it. The changes made to repository.ts can be divided into two parts:

  1. Consume the workspaces param and pass it to serializer / make serializer consume workspaces from input params or from .kibana index.
  2. Some preflight check on workspaces field and target workspace, so that the workspaces field won't be overwrite by accident.

For part 2, I think it is doable and has much benefit in the long term, will try to make the refact.

@SuZhou-Joe SuZhou-Joe marked this pull request as draft October 17, 2023 09:52
Signed-off-by: SuZhou-Joe <[email protected]>
@SuZhou-Joe
Copy link
Member Author

SuZhou-Joe commented Oct 18, 2023

@zengyan-amazon I did the refactor and you can find the changes in SuZhou-Joe#8 , please take a look to see if it is what you want, if so I will merge the refactor to this branch.

wrapper side change: https://github.com/SuZhou-Joe/OpenSearch-Dashboards/pull/8/files#diff-a2da6f3004def70510d19526890955d7480de07b173b72af04bcfa2f574d943e
repository change: https://github.com/SuZhou-Joe/OpenSearch-Dashboards/pull/8/files#diff-01c073903639e29be8562ed0de79fae4084a953304672d1e6bdb3fd7fef06f11

@SuZhou-Joe
Copy link
Member Author

Closed because of #5949.

@SuZhou-Joe SuZhou-Joe closed this Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Workspace] Optional workspaces params in repository
5 participants