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

feat: Leverage Imperative's event handling capabilities and more... #2994

Merged
merged 37 commits into from
Aug 1, 2024

Conversation

zFernand0
Copy link
Member

@zFernand0 zFernand0 commented Jul 9, 2024

Proposed changes

  • Leveraged the Imperative APIs to subscribe to client events.
  • Added onVaultUpdate and onCredMgrUpdate for extenders.
  • Leveraged SDK fix to CredentialManagerOverride APIs to properly disable the credential manager
  • Added return types to most interfaces and tests
  • Fixed all lint warnings

Release Notes

Milestone: 3.x.0

Changelog: Enhancement: Added onVaultUpdate and onCredMgrUpdate for extenders.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds or improves functionality)
  • 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):

Further comments

These changes where demoed during today's (July 9th, 2024) standup call 😋

Also, this PR is dependent on:

Signed-off-by: zFernand0 <[email protected]>
Copy link

codecov bot commented Jul 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.87%. Comparing base (2950dfa) to head (6f137e0).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2994      +/-   ##
==========================================
+ Coverage   92.82%   92.87%   +0.04%     
==========================================
  Files         107      107              
  Lines       10887    10913      +26     
  Branches     2280     2339      +59     
==========================================
+ Hits        10106    10135      +29     
+ Misses        779      776       -3     
  Partials        2        2              

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

Signed-off-by: zFernand0 <[email protected]>
@zFernand0 zFernand0 linked an issue Jul 23, 2024 that may be closed by this pull request
@zFernand0 zFernand0 linked an issue Jul 23, 2024 that may be closed by this pull request
Base automatically changed from update-sdks to next July 23, 2024 22:15
@zFernand0 zFernand0 changed the base branch from next to fix/ext-refresh-errors July 24, 2024 12:34
@zFernand0 zFernand0 changed the base branch from fix/ext-refresh-errors to next July 24, 2024 12:34
@zFernand0 zFernand0 changed the title feat: Leverage Imperative's Event Handling Capabilities feat: Leverage Imperative's Event Handling Capabilities and more... Jul 24, 2024
@zFernand0 zFernand0 changed the title feat: Leverage Imperative's Event Handling Capabilities and more... feat: Leverage Imperative's event handling capabilities and more... Jul 24, 2024
Copy link
Contributor

@anaxceron anaxceron left a comment

Choose a reason for hiding this comment

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

Made a couple of comments, please review 😺

packages/zowe-explorer-api/CHANGELOG.md Outdated Show resolved Hide resolved
packages/zowe-explorer-api/CHANGELOG.md Outdated Show resolved Hide resolved
@zFernand0
Copy link
Member Author

zFernand0 commented Jul 26, 2024

This looks good. Would it be possible to not show "Vault change detected. Refreshing profiles" when we change the password from Zowe Explorer?

It is theoretically possible to hide the info message (and avoid the extra refreshAll operation that happens behind the scenes). I can think of a couple of ways to do this.

  1. Give the watcher's callback function a sense of "self" by allowing it to receive a parameter. (imperative changes)
  2. Save in a "global" variable that it it just Zowe Explorer doing the password change. (only ZE changes)
  3. Disable the watcher momentarily right before the saving credentials and enable right after. (only ZE changes)
  4. Not show any info message and just refresh the tree providers and credentials behind the scenes. (only ZE changes)

I like option 4 😋


Also, should the notification be a prompt to refresh (with a yes/no button) or do we feel that it's ok to silently refresh?
I'm posting this question just for people's consideration, but I believe the current implementation (silent-refresh) is fine.

Here is my reasoning:

  • I feel like it's ok to silently refresh in the case of credentials being updated on the PC's vault. Even though the trigger occurred from another application, it still happened on your local PC, meaning that most likely you modified the credentials via the CLI (or some custom Imperative-SDK script or application) in which case you were not working directly with Zowe Explorer right in that moment.
  • Now, let's consider the credential manager being updated.
    • If you are in an environment where your Zowe client configuration is coming from a shared network, i.e. your CLI plug-ins and .zowe/settings are centralized and could be updated by administrators, you should be presented with the prompt to take action if the Kubernetes (K8s) plug-in was installed. Let's call this direction: CLI->K8s. However, you won't be prompted to take action if the change is the other way around, i.e. you were all using the K8s plug-in (and VSCE) and it (the .zowe/settings/imeprative.json#CredentialManager) is changed to the @zowe/cli, in which case you may loose previously stored credentials. Let's call this direction: K8s->CLI.
      Note: Your admins should ideally realize that changing credential managers will impact end users and should send a notice or provide a migration script.
    • If you are not in a centralized environment, i.e. everything happens on your local PC, the prompt will trigger for direction: CLI->K8s as expected. In the other direction: K8s-> CLI , the same note above applies to you since you are the admin.

zFernand0 and others added 5 commits July 26, 2024 06:25
Signed-off-by: Andrew W. Harn <[email protected]>
feat: Prevent base profile name collisions by default on new configuration files
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

Looks pretty good, thanks @zFernand0!

.eslintrc.yaml Show resolved Hide resolved
packages/zowe-explorer-api/src/extend/IRegisterClient.ts Outdated Show resolved Hide resolved
packages/zowe-explorer/CHANGELOG.md Show resolved Hide resolved
packages/zowe-explorer/src/trees/shared/SharedInit.ts Outdated Show resolved Hide resolved
packages/zowe-explorer/src/trees/shared/SharedInit.ts Outdated Show resolved Hide resolved
packages/zowe-explorer/src/utils/ProfilesUtils.ts Outdated Show resolved Hide resolved
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @zFernand0!

@JTonda JTonda removed the request for review from awharn July 30, 2024 15:08
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
4.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Copy link
Member

@traeok traeok left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks Fernando for the enhancement and for cleaning up the codebase 😋

@zFernand0 zFernand0 merged commit 49951e8 into next Aug 1, 2024
11 of 12 checks passed
@zFernand0 zFernand0 deleted the ext-not branch August 1, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

Update ZE to use imperative's APIs for updating imperative.json
6 participants