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

Update profile watchers and utilities to use Zowe SDK enhancements #3296

Merged
merged 16 commits into from
Nov 19, 2024

Conversation

t1m0thyj
Copy link
Member

@t1m0thyj t1m0thyj commented Nov 6, 2024

Proposed changes

⚠️ Depends on Zowe CLI PRs:

Release Notes

Milestone: 3.1.0

Changelog:

  • Fixed an issue where editing a team config file or updating credentials in OS vault could trigger multiple events for a single action.
  • Updated Zowe SDKs to 8.8.2 for technical currency.

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

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 93.82716% with 5 lines in your changes missing coverage. Please review.

Project coverage is 92.99%. Comparing base (7143336) to head (2a4bf53).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
packages/zowe-explorer/src/utils/ProfilesUtils.ts 94.23% 3 Missing ⚠️
...ages/zowe-explorer/src/trees/shared/SharedUtils.ts 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3296      +/-   ##
==========================================
- Coverage   92.99%   92.99%   -0.01%     
==========================================
  Files         116      116              
  Lines       12069    12044      -25     
  Branches     2738     2760      +22     
==========================================
- Hits        11224    11200      -24     
+ Misses        843      842       -1     
  Partials        2        2              

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


🚨 Try these New Features:

Copy link

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

anaxceron
anaxceron previously approved these changes Nov 14, 2024
Signed-off-by: Timothy Johnson <[email protected]>
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.

changes make sense to me 😋


will approve after testing! 😋


I know some of these lint issues weren't introduced by this PR.
However, if it's too much trouble, could we address them here?
https://github.com/zowe/zowe-explorer-vscode/actions/runs/11855312035/job/33039267040?pr=3296#step:6:129

packages/zowe-explorer/src/utils/ProfilesUtils.ts Outdated Show resolved Hide resolved
packages/zowe-explorer/src/utils/ProfilesUtils.ts Outdated Show resolved Hide resolved
Signed-off-by: Timothy Johnson <[email protected]>
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! 😋

The changes make the behavior a lot better since we don't refresh ZE as many times now, but I'm wondering if we are refreshing it too little now 😋

I'll like to discuss a few things before approving though.

ZoweLogger.info(vscode.l10n.t("Team config file created, refreshing Zowe Explorer."));
void SharedActions.refreshAll();
ZoweExplorerApiRegister.getInstance().onProfilesUpdateEmitter.fire(Validation.EventType.CREATE);
}, 100) // eslint-disable-line no-magic-numbers
Copy link
Member

Choose a reason for hiding this comment

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

Should we make the delay optional and default it to 100?

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.

One more thing, and sorry for being picky.
There are changed to l10n files coming out of the prepublish script

  • pnpm --filter vscode-extension-for-zowe vscode:prepublish

anaxceron
anaxceron previously approved these changes Nov 15, 2024
Signed-off-by: Timothy Johnson <[email protected]>
zFernand0
zFernand0 previously approved these changes Nov 15, 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.

LGTM! 😋

Thanks for addressing my dumb questions 😅

JillieBeanSim
JillieBeanSim previously approved these changes Nov 15, 2024
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.

LGTM! thanks @t1m0thyj

@t1m0thyj t1m0thyj added this to the v3.1.0 milestone Nov 15, 2024
@JillieBeanSim JillieBeanSim dismissed stale reviews from zFernand0 and themself via 2a4bf53 November 15, 2024 20:09
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.

Thanks Timothy for the updates and refactoring - I had a question about testing SharedUtils.debounce, but I tested the config watcher and everything LGTM

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 some comments on a changelog

@@ -6,7 +6,7 @@ All notable changes to the "zowe-explorer-api" extension will be documented in t

### New features and enhancements

- Update Zowe SDKs to `8.2.0` to get the latest enhancements from Imperative.
- Update Zowe SDKs to `8.8.2` to get the latest enhancements from Imperative.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an issue or PR link we could provide here?

@@ -27,7 +27,9 @@ All notable changes to the "vscode-extension-for-zowe" extension will be documen
- Fixed an issue where opening a PDS member after renaming an expanded PDS resulted in an error. [#3314](https://github.com/zowe/zowe-explorer-vscode/issues/3314)
- Fixed issue where users were not prompted to enter credentials if a 401 error was encountered when opening files, data sets or spools in the editor. [#3197](https://github.com/zowe/zowe-explorer-vscode/issues/3197)
- Fixed issue where profile credential updates or token changes were not reflected within the filesystem. [#3289](https://github.com/zowe/zowe-explorer-vscode/issues/3289)
- Fixed issue to update the success message when changing authentication from token to basic through the 'Change Authentication' option.
- Fixed issue to update the success message when changing authentication from token to basic through the 'Change Authentication' option. [#3316](https://github.com/zowe/zowe-explorer-vscode/pull/3316)
- Fixed an issue where editing a team config file or updating credentials in OS vault could trigger multiple events for a single action. [#3296](https://github.com/zowe/zowe-explorer-vscode/pull/3296)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Please write out "config"
  • Would it be appropriate to add an article here?: " ... updating credentials in the OS vault ... "
  • Should we explain what type of "vault" we're talking about?: " ... the OS credentials/security/kitten vault ..."

@JillieBeanSim JillieBeanSim merged commit d5dc428 into main Nov 19, 2024
26 of 27 checks passed
@JillieBeanSim JillieBeanSim deleted the fix/misc-profile-issues branch November 19, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

5 participants