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: Expose local storage through access control facility #3299

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

traeok
Copy link
Member

@traeok traeok commented Nov 6, 2024

Proposed changes

Resolves #3180

Dev doc: https://github.com/zowe/zowe-explorer-vscode/wiki/Local-Storage

How to test

  • Build a VSIX for Zowe Explorer using this PR and install it in VS Code
  • Checkout the local-storage-sample branch on https://github.com/zowe/zowe-client-samples
  • cd vscode-extension-samples/local-storage-sample && npm install && npm run compile
  • After the sample build has finished, click on the Run & Debug panel on the VS Code Side Bar
  • Select "Run Extension" from the tasks list at the top and select the Play ▶ button
  • Notice the notifications for the sample extension:
    • The first notification shown is for the list of writable keys
    • 2nd notification: list of readable keys
    • 3rd notification: the value for zowe.ds.history (a readable key)
    • 4th notification: trying to access a key that isn't exposed

Release Notes

Milestone: 3.1.0

Changelog:

  • Exposed read and write access to local storage keys for Zowe Explorer extenders. #3180

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
    • Fixed issue where workspace settings were merged into wrong local storage layer during migration
  • Enhancement (non-breaking change which adds or improves functionality)
    • Added support for ZoweLocalStorage to save in a workspace
    • Added the local storage access facility for extenders
  • Breaking change (a change that would cause existing functionality to not work as expected)
  • Documentation (Markdown, README updates)
  • Other (please specify above in "Proposed changes" section)

Checklist

General

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • All PR dependencies have been merged and published (if applicable)
  • A GIF or screenshot is included in the PR for visual changes
  • The pre-publish command has been executed:
    • v2 and below: yarn workspace vscode-extension-for-zowe vscode:prepublish
    • v3: pnpm --filter vscode-extension-for-zowe vscode:prepublish

Code coverage

  • There is coverage for the code that I have added
  • I have added new test cases and they are passing
  • I have manually tested the changes

Deployment

  • I have added developer documentation (if applicable)
  • Documentation should be added to Zowe Docs
    • If you're an outside contributor, please post in the #zowe-doc Slack channel to coordinate documentation.
    • Otherwise, please check with the rest of the squad about any needed documentation before merging.
  • These changes may need ported to the appropriate branches (list here):

Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
@traeok traeok added this to the v3.1.0 milestone Nov 6, 2024
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 96.82540% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.99%. Comparing base (16963db) to head (346f63b).

Files with missing lines Patch % Lines
...ckages/zowe-explorer/src/tools/ZoweLocalStorage.ts 95.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3299   +/-   ##
=======================================
  Coverage   92.98%   92.99%           
=======================================
  Files         116      116           
  Lines       12051    12096   +45     
  Branches     2768     2774    +6     
=======================================
+ Hits        11206    11249   +43     
- Misses        843      845    +2     
  Partials        2        2           

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

Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
@traeok traeok force-pushed the refactor/expose-local-storage branch from 1ef63b5 to 52b421f Compare November 7, 2024 15:06
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
@traeok traeok changed the title Refactor/expose local storage refactor: Expose local storage through access control facility Nov 7, 2024
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
@traeok traeok marked this pull request as ready for review November 8, 2024 15:59
Copy link

github-actions bot commented Nov 8, 2024

📅 Suggested merge-by date: 11/22/2024

anaxceron
anaxceron previously approved these changes Nov 8, 2024
@traeok traeok mentioned this pull request Nov 11, 2024
16 tasks
Copy link
Contributor

@JillieBeanSim JillieBeanSim left a comment

Choose a reason for hiding this comment

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

thanks for working on this @traeok
I do see that it wiped my zowe history which may need investigating, we wouldn't want to have that happen to customers. I didn't see the pop up described for the sample, I received the migration pop up with restart button.

not sure if someone else can replicate the issue I had seen or if just with me.

Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
@traeok
Copy link
Member Author

traeok commented Nov 13, 2024

thanks for working on this @traeok I do see that it wiped my zowe history which may need investigating, we wouldn't want to have that happen to customers. I didn't see the pop up described for the sample, I received the migration pop up with restart button.

not sure if someone else can replicate the issue I had seen or if just with me.

Thanks @JillieBeanSim for catching this, I realized that we need to use a defaultValue of undefined when accessing workspaceState now that our local storage class supports it. That way, if no workspace value was found, it will use the global value OR the default value for that key. Addressed in bd10595

anaxceron
anaxceron previously approved these changes Nov 13, 2024
zFernand0
zFernand0 previously approved these changes Nov 14, 2024
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

TL;DR: LGTM! 😋
Sorry for the long text below 😅


Question, should we merge the local-storage-sample branch into zowe-client-samples # main?
Also, should there be an activationEvent (maybe onStartupFinished)? Without it, I didn't get any notifications since the local-storage sample extension wouldn't activate 😋

Here is what I got when I added the activationEvent
image

And as expected, the notifications are in reverse order 😋


It seems like could benefit from running prettier on this branch 😋

modified:   packages/zowe-explorer/__tests__/__unit__/trees/dataset/DatasetFSProvider.unit.test.ts
modified:   packages/zowe-explorer/l10n/bundle.l10n.json
modified:   packages/zowe-explorer/l10n/poeditor.json

For those testing, please make sure that your .npmrc isn't pointing the @zowe scoped registry to something other than the public NPM registry 😅

npm error code E404
npm error 404 Not Found - GET https://zowe.jfrog.io/zowe/api/npm/npm-local-release/@zowe%2fzowe-explorer-api
npm error 404
npm error 404  '@zowe/zowe-explorer-api@^3.0.0' is not in this registry.
npm error 404
npm error 404 Note that you can also install from a
npm error 404 tarball, folder, http url, or git url.

Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
@traeok traeok dismissed stale reviews from zFernand0 and anaxceron via e63886d November 14, 2024 14:27
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Review/QA
Development

Successfully merging this pull request may close these issues.

Allow users to access persistent data from Zowe Explorer local storage
4 participants