-
Notifications
You must be signed in to change notification settings - Fork 92
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
Fix 401 error when opening PDS members after cred change #3157
Conversation
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3157 +/- ##
==========================================
+ Coverage 92.81% 92.97% +0.16%
==========================================
Files 113 113
Lines 11661 11661
Branches 2500 2502 +2
==========================================
+ Hits 10823 10842 +19
+ Misses 836 817 -19
Partials 2 2 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
📅 Suggested merge-by date: 10/15/2024 |
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.
LGTM! 😋
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.
If other applications are running into this problem, they could use this technique as an example.
At least until the enhancement below is implemented:
Thanks @t1m0thyj 🥳
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
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: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
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.
LGTM! 😋
left some small comments, but nothing urgent 😅
/** | ||
* Use Dataset-specific tree node for children. | ||
*/ | ||
children?: IZoweDatasetTreeNode[]; |
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.
should we deprecate getChildren()
of do we still need an async approach to get them? 😋
Same for USS
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 believe we still need an async approach to fetch items from the mainframe when we want to update the tree 😋
I am now getting stuck in a loop not able to update credentials when 401 detected, seeing the same with zosmf profiles. Screen.Recording.2024-10-07.at.4.43.28.PM.movsearch was inplace, I made credentials invalid with right-click update creds options and see this when going to expand data set. expected to be prompted for valid creds but wasn't. |
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
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 @JillieBeanSim and @traeok for testing this PR! I believe both issues should be fixed now - the Update Credentials loop and children not refreshing.
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
74b97fa
to
ad07311
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 fixing previous issue with the prompt but I'm sorry to say that I am back to the original issue of the error in the text file 😢
Screen.Recording.2024-10-08.at.9.50.00.AM.mov
packages/zowe-explorer/__tests__/__unit__/trees/job/ZoweJobNode.unit.test.ts
Fixed
Show fixed
Hide fixed
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
9b65486
to
6809327
Compare
This behavior happens in V3 because opening files in the editor is handled by the FileSystemProvider, so we'll have to add some logic to the FSP to handle 401 errors if we want it show pop ups instead. I've created a separate issue to track this: #3197 Since this PR fixes the original issue #3150 where credentials failed to update on member nodes, I'd like to get it included with v3.0.1 if possible 😋 |
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 @t1m0thyj for the fix
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.
LGTM! 😋
Thanks for addressing the feedback 😅
Proposed changes
ZE:
ZE API:
ZoweTreeNode.setProfileToChoice
function so that it propagates profile changes to its child nodes. #3150Release Notes
Milestone: v3 GA (or v3.1 if needed)
Changelog: See
Proposed changes
Types of changes
Checklist
General
yarn workspace vscode-extension-for-zowe vscode:prepublish
pnpm --filter vscode-extension-for-zowe vscode:prepublish
Code coverage
Deployment