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

Refactor: move workspace logic from repository to saved objects client wrapper #248

Merged

Conversation

SuZhou-Joe
Copy link
Collaborator

@SuZhou-Joe SuZhou-Joe commented Feb 22, 2024

Description

Move workspace check conflict related logic to a saved objects client wrapper so that the logic can be turned off once workspace plugin is disabled, introducing as less as possible effect on OSD core.

Issues Resolved

Screenshot

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

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
@SuZhou-Joe SuZhou-Joe changed the title Refactor/move workspace logic to wrapper Refactor: move workspace logic from repository to saved objects client wrapper Feb 22, 2024
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>
Copy link
Owner

@ruanyl ruanyl left a comment

Choose a reason for hiding this comment

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

Left a comment, nice refactor!

Copy link
Owner

Choose a reason for hiding this comment

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

Can we encapsulate this wrapper with the existing WorkspaceSavedObjectsClientWrapper? This may save some API calls to wrapperOptions.client.get and wrapperOptions.client.bulkGet

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, sounds reasonable. Will take a look then.

Copy link
Collaborator Author

@SuZhou-Joe SuZhou-Joe Feb 28, 2024

Choose a reason for hiding this comment

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

Did some research and it seems that these two wrappers are controlled by different flags: workspace.enabled and workspace.permission.enabled. I am afraid combining these two wrappers as one may lead to complex if/else. And as we will merge the conflict wrapper before permission check wrapper, I'd like to raise a following issue to optimize the code if we have more bandwidth on 2.13.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if they are for different purpose and can be used independently, better to have separate wrappers

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>
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>
@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2024

Codecov Report

Attention: Patch coverage is 67.79221% with 124 lines in your changes are missing coverage. Please review.

Project coverage is 66.75%. Comparing base (dc240eb) to head (25c32e5).
Report is 127 commits behind head on workspace.

Files Patch % Lines
...erver/saved_objects/import/create_saved_objects.ts 28.00% 14 Missing and 4 partials ⚠️
packages/osd-pm/src/utils/project.ts 26.08% 17 Missing ⚠️
packages/osd-pm/src/utils/scripts.ts 0.00% 15 Missing ⚠️
...rces/datasource_selector/datasource_selectable.tsx 68.29% 11 Missing and 2 partials ⚠️
src/core/server/saved_objects/routes/import.ts 0.00% 5 Missing ⚠️
...rver/saved_objects/routes/resolve_import_errors.ts 0.00% 5 Missing ⚠️
src/dev/build/lib/fs.ts 44.44% 5 Missing ⚠️
src/core/server/http/router/response_adapter.ts 0.00% 2 Missing and 2 partials ⚠️
...d_objects/import/check_conflict_for_data_source.ts 84.00% 1 Missing and 3 partials ⚠️
...cross_compatibility/cross_compatibility_service.ts 90.32% 2 Missing and 1 partial ⚠️
... and 21 more
Additional details and impacted files
@@              Coverage Diff              @@
##           workspace     #248      +/-   ##
=============================================
+ Coverage      66.24%   66.75%   +0.51%     
=============================================
  Files           3438     3337     -101     
  Lines          66264    64801    -1463     
  Branches       10694    10453     -241     
=============================================
- Hits           43899    43261     -638     
+ Misses         19757    18952     -805     
+ Partials        2608     2588      -20     
Flag Coverage Δ
Linux_1 31.41% <7.66%> (+0.52%) ⬆️
Linux_2 55.22% <57.72%> (-0.21%) ⬇️
Linux_3 41.74% <39.25%> (-1.03%) ⬇️
Linux_4 34.88% <12.07%> (+0.35%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -81,7 +81,7 @@ export class SavedObjectsUtils {
saved_objects: [],
});

public static filterWorkspacesAccordingToBaseWorkspaces(
public static filterWorkspacesAccordingToSourceWorkspaces(
Copy link
Owner

Choose a reason for hiding this comment

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

Seems this function is only used by src/plugins/workspace/server/saved_objects/saved_objects_wrapper_for_check_workspace_conflict.ts, I think we can move it there so we don't need to bother this util class in core

/**
* workspaces the objects belong to, will only be used when overwrite is enabled.
*/
workspaces?: string[];
Copy link
Owner

Choose a reason for hiding this comment

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

Isn't workspaces already specified in the options? SavedObjectsBaseOptions

Copy link
Collaborator

Choose a reason for hiding this comment

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

The SavedObjectsBulkCreateObject doesn't extends SavedObjectsBaseOptions.

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
@SuZhou-Joe SuZhou-Joe merged commit 415605f into ruanyl:workspace Feb 29, 2024
34 of 36 checks passed
@opensearch-workspace-development

The backport to workspace-pr-integr failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-workspace-pr-integr workspace-pr-integr
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-workspace-pr-integr
# Create a new branch
git switch --create backport/backport-248-to-workspace-pr-integr
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 415605f437ed138d4d8f5c08fa67a2b905ece4ff
# Push it to GitHub
git push --set-upstream origin backport/backport-248-to-workspace-pr-integr
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-workspace-pr-integr

Then, create a pull request where the base branch is workspace-pr-integr and the compare/head branch is backport/backport-248-to-workspace-pr-integr.

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.

5 participants