-
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][Feature]Setup workspace skeleton and implement basic CRUD API #5075
[Workspace][Feature]Setup workspace skeleton and implement basic CRUD API #5075
Conversation
52416b4
to
c632de5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5075 +/- ##
==========================================
+ Coverage 66.79% 66.81% +0.02%
==========================================
Files 3286 3288 +2
Lines 63129 63151 +22
Branches 10053 10054 +1
==========================================
+ Hits 42164 42196 +32
+ Misses 18490 18474 -16
- Partials 2475 2481 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks for raising the initial PR for workspace feature! I noticed this is a draft, but two things I'd suggest to add:
|
Sure, will add more test cases as well. |
216f10a
to
e58ab3f
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.
Thanks for opening this up!
A couple nits and couple of comments that I think should be considered.
Specifically file whitespacing, custom random generate id to pass to the creating of the workspace instead of relying on OpenSearch, and documentation on the APIs that are updated to accurately reflect usage.
Also, something to consider when implemented the more complicated APIs, Saved Objects API supports --compressed
so I'd imagine if I wanted to use this API I could use the same flag as well. Verified your stuff isn't impacted but just a headsup. Which can be extended on if we plan on workspaces being fully integrated with saved objects
/** | ||
* Generate URL friendly random ID | ||
*/ | ||
export const generateRandomId = (size: number) => { |
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.
is this a temp solution? i would utilize similar short url functionality with application or have gets work by name work if the purpose of this is to just generate url friendly randomly ids.
as i see it being used to generate the actual ID use to saved the document. To which point I would much rather rely on the id provided by OpenSearch on insert instead of building out a custom function configured to a length of 6.
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 workspaceId will be prepended to url in the following PR, so a uuid with a length of 32 will be a little bit too large for a url path or query, so we choose to make it shorter by using this method.
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 workspaceId will be prepended to url in the following PR, so a uuid with a length of 32 will be a little bit too large for a url path or query, so we choose to make it shorter by using this method.
That is pretty standard for the application. For example:
https://playground.opensearch.org/app/dashboards#/view/7adfa750-4c81-11e8-b3d7-01146121b73d?_g=(filters:!(),refreshInterval:(pause:!f,value:900000),time:(from:now-24h,to:now))&_a=(description:'Analyze%20mock%20flight%20data%20for%20OpenSearch-Air,%20Logstash%20Airways,%20OpenSearch%20Dashboards%20Airlines%20and%20BeatsWest',filters:!(),fullScreenMode:!f,options:(hidePanelTitles:!f,useMargins:!t),query:(language:kuery,query:''),timeRestore:!t,title:'%5BFlights%5D%20Global%20Flight%20Dashboard',viewMode:view)`.
The url is pretty large and is what we utilize for global and app state so this can get even longer. I would be worried about potential conflicts. Custom generate ID functions tend to not actually be as random as they imply so it would be a lot safer to defer to OpenSearch to index and provide you with a verified unique ID.
If the length of the URL can be shorten utilize the short URL function
.
Or you can also make titles unique ids as well. I believe index patterns are the same way.
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.
I c, but as workspace will be a new context for our url, like later we will visit all existing routes by [https://playground.opensearch.org/w/${uuid_for_current_workspace}/app/dashboards], in this case, a shorten url will make it more user friendly for user to see which routes they are viewing.
Even if there is a conflict between a new-created workspaceId and an existing one, though the odds is very small (10000 / 256^6), there will be a error toast and user will be able to re-click the create button to create. This is a tradeoff
we want to make.
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.
(10000 / 256^6)
It's likely less than that:
https://nodejs.org/api/crypto.html#cryptorandombytessize-callback
cryptographically strong pseudorandom data
So it will likely be based on a randomly selected seed value set by the OS.
[https://playground.opensearch.org/w/${uuid_for_current_workspace}/app/dashboards],
Apologies, I wasn't aware of this. I believe this might have a larger implication to the application and how the application operates and will probably increase the scope of this project significantly.
Few examples off the top of my head:
- OSD starts up and discovers what plugins that are installed. It then takes the bundles for plugins and pushes under url/app/pluginName. So the current navigation system will have to be reworked. Because it will depend on url/app/pluginName to make the appropriate HTTP calls to download the bundles.
- History will have to be considered as well. Parts of the application and external plugins can be relying on the URL to handle history so if only parts of the application have the workspace route then they can get in a bad state.
- State management is managed by the URL via
_a
(for app) and_g
so this might be impacted and you will have to modify existing OSD URL storage functions as some functions might be slicing up the route based onurl/{base_path}/app/plugin
- APIs get hosted under url/api/pluginName
make it more user friendly for user to see which routes they are viewing
Likely the URLs are obfuscated anyways depending on the size of your screen (mobile users). And if the goal is to be user friendly I think I would just go all the way and utilize the title as the unique id.
tl;dr: So overall, I think that might add a lot of scope creep to add that and should be a proposal. To the point where I wouldn't want to consider that as a reason to keep in shorten Ids when we could add a new state like _w
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.
However things are difference between index patterns because workspace's title can be updated after a workspace has been created.
As for prepending workspaceId into our url path, there will be a RFC to discuss the options and we can forget that on this PR.
So what about we using a larger size, for example 12 maybe, as our workspaceId? As a length of 32 is too long for a url, especially when user copy a object detail page url, there will be a length of 32 to be used for workspaceId and a length of 32 to be used for object Id.
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.
I don't think using a 6 character ID doesn't make big difference than a 32 bytes UUID. and agree with @kavilla that if you really want to make it friendly, you can use user specified friendly id as the workspace document id instead using auto generated ones.
And regarding prepending the workspace id in the url path, I think Kibana namespace does it, but do we compare other options like using query parameter?
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.
@SuZhou-Joe any thinking around this?
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.
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
… API (#5075) * feature: setup workspace skeleton and implement basic CRUD API on workspace Signed-off-by: Zhou Su <suzhou@dev-dsk-suzhou-2a-8ce7a7a7.us-west-2.amazon.com> * feat: remove useless required plugins and logger typo Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: setup public side skeleton Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * temp: add unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add function test for workspace CRUD routes Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: use saved objects client instead of internal repository Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update CHANGELOG Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: exclude permission check wrapper Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add integration test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add configuration Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: enable workspace flag when run workspace related test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimization according to PR comments Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add JSDoc for workspace client Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * Update src/plugins/workspace/server/integration_tests/routes.test.ts Co-authored-by: Josh Romero <rmerqg@amazon.com> Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove hard-coded delay Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: Only allow workspace CRUD APIs to modify workspace metadata. Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add integration test for new changes Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: Zhou Su <suzhou@dev-dsk-suzhou-2a-8ce7a7a7.us-west-2.amazon.com> Signed-off-by: SuZhou-Joe <suzhou@amazon.com> Co-authored-by: Zhou Su <suzhou@dev-dsk-suzhou-2a-8ce7a7a7.us-west-2.amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit eeb3251) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
… API (opensearch-project#5075) * feature: setup workspace skeleton and implement basic CRUD API on workspace Signed-off-by: Zhou Su <suzhou@dev-dsk-suzhou-2a-8ce7a7a7.us-west-2.amazon.com> * feat: remove useless required plugins and logger typo Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: setup public side skeleton Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * temp: add unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add function test for workspace CRUD routes Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: use saved objects client instead of internal repository Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update CHANGELOG Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: exclude permission check wrapper Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add integration test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add configuration Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: enable workspace flag when run workspace related test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimization according to PR comments Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add JSDoc for workspace client Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * Update src/plugins/workspace/server/integration_tests/routes.test.ts Co-authored-by: Josh Romero <rmerqg@amazon.com> Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove hard-coded delay Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: Only allow workspace CRUD APIs to modify workspace metadata. Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add integration test for new changes Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: Zhou Su <suzhou@dev-dsk-suzhou-2a-8ce7a7a7.us-west-2.amazon.com> Signed-off-by: SuZhou-Joe <suzhou@amazon.com> Co-authored-by: Zhou Su <suzhou@dev-dsk-suzhou-2a-8ce7a7a7.us-west-2.amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com>
Based off of #5409 (comment) I think this is okay its not in 2.x yet but the backport might be difficult in the future. @SuZhou-Joe @ruanyl @xluo-aws do you have information to share about status of workspace feature. And is it okay that I removed the 2.12 label? |
Rocky, thanks for the reminder, the new target release is 2.14. |
The backport to
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-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-5075-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 eeb325163a65c750405714ea6674ce26607608d5
# Push it to GitHub
git push --set-upstream origin backport/backport-5075-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x Then, create a pull request where the |
… API (#5075) * feature: setup workspace skeleton and implement basic CRUD API on workspace Signed-off-by: Zhou Su <suzhou@dev-dsk-suzhou-2a-8ce7a7a7.us-west-2.amazon.com> * feat: remove useless required plugins and logger typo Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: setup public side skeleton Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * temp: add unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add function test for workspace CRUD routes Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: use saved objects client instead of internal repository Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update CHANGELOG Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: exclude permission check wrapper Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add integration test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add configuration Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: enable workspace flag when run workspace related test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimization according to PR comments Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add JSDoc for workspace client Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * Update src/plugins/workspace/server/integration_tests/routes.test.ts Co-authored-by: Josh Romero <rmerqg@amazon.com> Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove hard-coded delay Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: Only allow workspace CRUD APIs to modify workspace metadata. Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add integration test for new changes Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: Zhou Su <suzhou@dev-dsk-suzhou-2a-8ce7a7a7.us-west-2.amazon.com> Signed-off-by: SuZhou-Joe <suzhou@amazon.com> Co-authored-by: Zhou Su <suzhou@dev-dsk-suzhou-2a-8ce7a7a7.us-west-2.amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit eeb3251) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
… API (#5075) (#6381) * feature: setup workspace skeleton and implement basic CRUD API on workspace Signed-off-by: Zhou Su <suzhou@dev-dsk-suzhou-2a-8ce7a7a7.us-west-2.amazon.com> * feat: remove useless required plugins and logger typo Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: setup public side skeleton Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * temp: add unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add function test for workspace CRUD routes Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: use saved objects client instead of internal repository Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update CHANGELOG Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: exclude permission check wrapper Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add integration test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add configuration Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: enable workspace flag when run workspace related test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimization according to PR comments Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add JSDoc for workspace client Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * Update src/plugins/workspace/server/integration_tests/routes.test.ts Co-authored-by: Josh Romero <rmerqg@amazon.com> Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove hard-coded delay Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: Only allow workspace CRUD APIs to modify workspace metadata. Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add integration test for new changes Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: Zhou Su <suzhou@dev-dsk-suzhou-2a-8ce7a7a7.us-west-2.amazon.com> Signed-off-by: SuZhou-Joe <suzhou@amazon.com> Co-authored-by: Zhou Su <suzhou@dev-dsk-suzhou-2a-8ce7a7a7.us-west-2.amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit eeb3251) 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 is the initial PR for workspace, as we know workspace will be a new function that provides:
And before all of the features mentioned above, we need to setup the workspace as a core plugin.
This PR includes:
workspace
.workspace
.Issues Resolved
#4945
Screenshot
The changes introduced by this PR is not related to UI change.
Testing the changes
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr