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] Add workspace id in basePath #6060

Merged

Conversation

SuZhou-Joe
Copy link
Member

@SuZhou-Joe SuZhou-Joe commented Mar 7, 2024

Description

In order to let users switch to specific workspace when paste a deep link from other users, we need to make url stateful with workspace info. This PR mainly introduce the capability to keep the workspace Id in url consistent with the workspace id in memory so that user can feel free to copy the url and share to others.

You can find all the options and discussions from the RFC issue: #5243 .

Issues Resolved

#6015

Screenshot

20240307174817818

Testing the changes

  • Using the branch to bootstrap
  • Enable workspace by adding workspace.enabled to opensearch_dashboards.yml file
  • Start OSD by using yarn start --no-base-path to make sure no random base path will be appended
  • Visit the OpenSearch Dashboards under foo workspace: http://localhost:5601/w/foo/app/dev_tools
  • You will find /w/foo params will always be there when you navigate between left navigation, making the url shareable.

Check List

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

@SuZhou-Joe SuZhou-Joe changed the title [Workspace]Add workspace id in basePath (#212) [Workspace] Add workspace id in basePath Mar 7, 2024
Copy link

codecov bot commented Mar 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.17%. Comparing base (df6de4e) to head (b4f0a87).
Report is 602 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6060      +/-   ##
==========================================
+ Coverage   67.16%   67.17%   +0.01%     
==========================================
  Files        3327     3328       +1     
  Lines       64415    64448      +33     
  Branches    10366    10376      +10     
==========================================
+ Hits        43262    43295      +33     
  Misses      18621    18621              
  Partials     2532     2532              
Flag Coverage Δ
Linux_1 31.74% <45.65%> (+0.01%) ⬆️
Linux_2 55.29% <100.00%> (+0.06%) ⬆️
Linux_3 44.75% <40.47%> (-0.01%) ⬇️
Linux_4 35.05% <30.95%> (-0.01%) ⬇️
Windows_1 31.77% <45.65%> (+0.01%) ⬆️
Windows_2 55.26% <100.00%> (+0.06%) ⬆️
Windows_3 44.76% <40.47%> (-0.01%) ⬇️
Windows_4 35.05% <30.95%> (-0.01%) ⬇️

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.

* feat: enable workspace id in basePath

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

* feat: add unit test

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

* feat: remove useless test object id

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

* feat: add unit test

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

* feat: add unit test

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

* feat: update snapshot

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

* feat: move formatUrlWithWorkspaceId to core/public/utils

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

* feat: remove useless variable

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

* feat: remove useless variable

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

* feat: optimization

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

* feat: optimization

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

* feat: optimization

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

* feat: move workspace/utils to core

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

* feat: move workspace/utils to core

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

* feat: update comment

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

* feat: optimize code

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

* feat: update unit test

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

* feat: optimization

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

* feat: add space under license

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

* fix: unit test

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]>
@ashwin-pc
Copy link
Member

@ashwin-pc I think the major concern left is the one that we may introduce an in-direct dependency in core to plugin (#6060 (comment)) , For now there are two options:

  1. Leave it as it is and add a TODO comment to optimize that part in the future with a following issue.
  2. Revert to the version I mentioned in the response, which will get clientBasePath no matter workspace is enabled or not. You can find the diff here: 64b3645#diff-f1218ada97a1395de9cad0d115290e84da30f1666d5092e86a12d92a486813b3 .

Which option do you perfer?

Think this goes back to my initial question, dont we already have access to the config in core? Here is where we inject the metadata from the config into the injected metadata (Link). Branding uses this to decide how to apply branding to the app. We then use this data in core to do things similar to what you are doing here. The difference is that this looks for the config value instead of the existence of a plugin and hence does not have a dependency on the plugin. This should solve both, the need for the front end workspaces component in this PR for rerouting and the selective workspace ID parsing

Signed-off-by: SuZhou-Joe <[email protected]>
@SuZhou-Joe
Copy link
Member Author

But there are some difference, config for branding is a core configuration while the config for workspace is a plugin configuration. Plugin configurations are transformed to uiPlugins, so workspace.enabled = true equals: workspace plugin can be found in uiPlugins list. If we try to find the real config workspace.enabled, we need to expose the raw config to browser side in server/render_service and I afraid that may expose too much info.

Signed-off-by: SuZhou-Joe <[email protected]>
@ruanyl
Copy link
Member

ruanyl commented Mar 12, 2024

@ashwin-pc I think the major concern left is the one that we may introduce an in-direct dependency in core to plugin (#6060 (comment)) , For now there are two options:

  1. Leave it as it is and add a TODO comment to optimize that part in the future with a following issue.
  2. Revert to the version I mentioned in the response, which will get clientBasePath no matter workspace is enabled or not. You can find the diff here: 64b3645#diff-f1218ada97a1395de9cad0d115290e84da30f1666d5092e86a12d92a486813b3 .

Which option do you perfer?

I tend to choose the 2nd approach. If someone tries to visit a workspace URL when workspace is disabled, I think it's fine as long as it returns 404.

if (!this.basePath) {
public remove = (path: string, prependOptions?: PrependOptions): string => {
const { withoutWorkspace } = prependOptions || {};
const basePath = withoutWorkspace ? this.basePath : this.get();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can rename the "workspace" in prepend and remove function as well^^

Copy link
Member Author

Choose a reason for hiding this comment

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

I was intending to rename the withoutWorkspace to withoutClientBasePath but I was thinking in the future, the clientBasePath may consist of workspace/dataSourceId/other stuff and prependOptions will have withoutWorkspace/withoutDataSourceId/withoutClientBasePath options accordingly so I kept them.

But for now, there is no such case so it has no harm to have only one withoutClientBasePath config in prependOptions, but will add some comment here.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks for the clarification^^

Signed-off-by: SuZhou-Joe <[email protected]>
This reverts commit 64b3645.

Signed-off-by: SuZhou-Joe <[email protected]>
This reverts commit 80bed72.

Signed-off-by: SuZhou-Joe <[email protected]>
@SuZhou-Joe SuZhou-Joe force-pushed the feature/stateful_url_path_official branch from a9159e4 to 3980cd7 Compare March 12, 2024 06:58
ruanyl
ruanyl previously approved these changes Mar 13, 2024
Signed-off-by: SuZhou-Joe <[email protected]>
@SuZhou-Joe
Copy link
Member Author

@ashwin-pc Reverted to option 2, could you please help to review here?

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -30,4 +30,22 @@ describe('Workspace plugin', () => {
coreStart.workspaces.currentWorkspaceId$.next('foo');
expect(coreStart.savedObjects.client.setCurrentWorkspace).toHaveBeenCalledWith('foo');
});

it('#setup when workspace id is in url and enterWorkspace return error', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('#setup when workspace id is in url and enterWorkspace return error', async () => {
it('should correctly set up WorkspacePlugin when workspace ID is present in the URL', async () => {

The current test title does not match whats being tested for. You dont seem to be testing for an error. This is just a suggestion based on what the test seems to be doing

Copy link
Member Author

@SuZhou-Joe SuZhou-Joe Mar 15, 2024

Choose a reason for hiding this comment

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

Thanks, it will be automatically addressed by #6154 . A mistake when splitting a large PR into several smaller PRs.

Comment on lines +90 to +98
/**
* prepend options
*
* withoutClientBasePath option will prepend a relative url with serverBasePath only.
* For now, clientBasePath is consist of:
* workspacePath, which has the pattern of /w/{workspaceId}.
*
* In the future, clientBasePath may have other parts but keep `withoutClientBasePath` for now to not over-design the interface,
*/
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding context here

@SuZhou-Joe SuZhou-Joe merged commit 3073926 into opensearch-project:main Mar 14, 2024
68 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 12, 2024
* [Workspace]Add workspace id in basePath (#212)

* feat: enable workspace id in basePath

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

* feat: add unit test

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

* feat: remove useless test object id

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

* feat: add unit test

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

* feat: add unit test

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

* feat: update snapshot

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

* feat: move formatUrlWithWorkspaceId to core/public/utils

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

* feat: remove useless variable

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

* feat: remove useless variable

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

* feat: optimization

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

* feat: optimization

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

* feat: optimization

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

* feat: move workspace/utils to core

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

* feat: move workspace/utils to core

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

* feat: update comment

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

* feat: optimize code

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

* feat: update unit test

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

* feat: optimization

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

* feat: add space under license

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

* fix: unit test

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

---------

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

* feat: add CHANGELOG

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

* feat: add feature flag check

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

* feat: make the pr smaller

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

* feat: optimize with a more strict check

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

* fix: unit test error

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

* feat: remove useless code

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

* feat: add a unit test case

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

* feat: better merge

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

* feat: rename the workspaceBasePath to clientBasePath

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

* fix: snapshot

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

* feat: rename withoutWorkspace to withoutClientBasePath

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

* Revert "feat: add feature flag check"

This reverts commit 64b3645.

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

* Revert "fix: unit test error"

This reverts commit 80bed72.

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

* feat: optimize comment and test cases description

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

* feat: optimize comment

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

---------

Signed-off-by: SuZhou-Joe <[email protected]>
(cherry picked from commit 3073926)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
SuZhou-Joe pushed a commit that referenced this pull request Apr 14, 2024
* [Workspace]Add workspace id in basePath (#212)

* feat: enable workspace id in basePath

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

* feat: add unit test

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

* feat: remove useless test object id

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

* feat: add unit test

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

* feat: add unit test

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

* feat: update snapshot

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

* feat: move formatUrlWithWorkspaceId to core/public/utils

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

* feat: remove useless variable

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

* feat: remove useless variable

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

* feat: optimization

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

* feat: optimization

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

* feat: optimization

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

* feat: move workspace/utils to core

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

* feat: move workspace/utils to core

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

* feat: update comment

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

* feat: optimize code

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

* feat: update unit test

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

* feat: optimization

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

* feat: add space under license

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

* fix: unit test

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

---------

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

* feat: add CHANGELOG

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

* feat: add feature flag check

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

* feat: make the pr smaller

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

* feat: optimize with a more strict check

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

* fix: unit test error

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

* feat: remove useless code

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

* feat: add a unit test case

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

* feat: better merge

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

* feat: rename the workspaceBasePath to clientBasePath

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

* fix: snapshot

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

* feat: rename withoutWorkspace to withoutClientBasePath

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

* Revert "feat: add feature flag check"

This reverts commit 64b3645.

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

* Revert "fix: unit test error"

This reverts commit 80bed72.

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

* feat: optimize comment and test cases description

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

* feat: optimize comment

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

---------

Signed-off-by: SuZhou-Joe <[email protected]>
(cherry picked from commit 3073926)
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>
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