-
Notifications
You must be signed in to change notification settings - Fork 93
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
[v2] Port fix for credentials not refreshed in ds tree and other misc profile fixes #3349
base: v2-lts
Are you sure you want to change the base?
Conversation
Signed-off-by: Timothy Johnson <[email protected]>
Signed-off-by: Timothy Johnson <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2-lts #3349 +/- ##
==========================================
+ Coverage 93.41% 93.45% +0.04%
==========================================
Files 105 105
Lines 11161 11154 -7
Branches 2367 2465 +98
==========================================
- Hits 10426 10424 -2
+ Misses 734 729 -5
Partials 1 1 ☔ View full report in Codecov by Sentry. |
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.
Hey @t1m0thyj thanks for porting these bugfixes. I did leave a comment below
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 port LGTM as-is! 🙏
Billie has a good point about adding a new API in a maintenance branch.
I'm not opposed to adding it, but also shared a theoretical alternative solution if we decide not to add it 🙏
We can wait for Timothy's input before we proceed with the PR though
Signed-off-by: Timothy Johnson <[email protected]>
d88d09e
to
16a26a5
Compare
Signed-off-by: Timothy Johnson <[email protected]>
Quality Gate failedFailed conditions |
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 porting these Timothy, the fixes LGTM!
- Fixed issue where Search operation did not prompt for credentials if profile contains expired token. [#2259](https://github.com/zowe/zowe-explorer-vscode/issues/2259) | ||
- Fixed issue where inactive status was not displayed for profiles loaded from Global Config. [#3134](https://github.com/zowe/zowe-explorer-vscode/issues/3134) |
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.
Suggestion: We'll probably want to move these entries to the bottom of this section as the existing entries are sorted chronologically
await cache.updateBaseProfileFileLogout(serviceProfile); | ||
} | ||
await cache.updateBaseProfileFileLogout(connOk ? baseProfile : serviceProfile); | ||
serviceProfile.profile = { ...serviceProfile.profile, tokenType: undefined, tokenValue: undefined }; |
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 noticed serviceProfile.profile
is reassigned as a new object - since this is in v2, I don't think this has any major side effects. However, it does make me wonder if we have similar logic in v3 and whether that logic works well with other parts of Zowe Explorer that keep profile references (such as tree nodes, filesystem) 🤔
No request for changes here, it's merely an observation 😋
Proposed changes
Ports #3111 and #3248 from main branch to v2-lts branch. Original PR description below:
Release Notes
Milestone: 2.18.1
Changelog:
Zowe Explorer
Zowe Explorer API
ProfilesCache.updateProfilesArrays
. UseProfilesCache.updateCachedProfile
instead, which handles updating credentials cached in memory whenautoStore
is false. #3120Types of changes
Checklist
General
yarn workspace vscode-extension-for-zowe vscode:prepublish
pnpm --filter vscode-extension-for-zowe vscode:prepublish
Code coverage
Deployment
Further comments