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

Fix credentials not refreshed in ds tree and misc profile related fixes #3111

Merged
merged 23 commits into from
Sep 25, 2024

Conversation

t1m0thyj
Copy link
Member

@t1m0thyj t1m0thyj commented Sep 12, 2024

Proposed changes

This PR resolves several issues related to profile management in V3:

How to Test
Test that profile changes are propagated:

  • Update credentials (user/password)
  • Login/logout with base profile
  • Login/logout with regular profile
  • All the above when autoStore=false

Release Notes

Milestone: 3.0.0

Changelog: TBD

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

Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
@traeok traeok linked an issue Sep 18, 2024 that may be closed by this pull request
Base automatically changed from fix/multi-apiml-logout to next September 19, 2024 15:00
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
@adam-wolfe adam-wolfe mentioned this pull request Sep 23, 2024
15 tasks
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Copy link

codecov bot commented Sep 23, 2024

Codecov Report

Attention: Patch coverage is 98.07692% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.80%. Comparing base (3cf3f66) to head (409bcd4).
Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
...ckages/zowe-explorer/src/configuration/Profiles.ts 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3111      +/-   ##
==========================================
+ Coverage   92.76%   92.80%   +0.03%     
==========================================
  Files         113      113              
  Lines       11658    11669      +11     
  Branches     2453     2594     +141     
==========================================
+ Hits        10815    10829      +14     
+ Misses        841      838       -3     
  Partials        2        2              

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

@t1m0thyj t1m0thyj linked an issue Sep 24, 2024 that may be closed by this pull request
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
@t1m0thyj t1m0thyj added the needs-ported Apply to issues or PRs that need ported label Sep 24, 2024
@t1m0thyj t1m0thyj modified the milestones: v3 pre-releases, v3.0.0 GA Sep 24, 2024
@t1m0thyj t1m0thyj changed the title WIP Fix credential desyncs across tree nodes Fix credentials not refreshed in ds tree and misc profile management fixes Sep 24, 2024
@t1m0thyj t1m0thyj changed the title Fix credentials not refreshed in ds tree and misc profile management fixes Fix credentials not refreshed in ds tree and misc profile related fixes Sep 24, 2024
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
@t1m0thyj t1m0thyj marked this pull request as ready for review September 24, 2024 03:50
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
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 these bugs @t1m0thyj
I did experience some strange behavior after updating credentials from failing one to working one and successfully did search and expand members but got the below error in text editor when trying to open file
Screenshot 2024-09-24 at 9 14 02 AM

Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
@t1m0thyj
Copy link
Member Author

@JillieBeanSim Thanks for catching this, I think I've fixed the issue but need to update some tests 😋

Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
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! 😋

Left a couple of comments but nothing that requires changes on this PR 😋

Nice to see the error icon for global profiles now, thank you!
image

A few quirks:

  • With autoStore: false and only the login regular profile
    • ✅ triggering the equivalent of updateCreds works fine with nested profiles
    • ❓ triggering the equivalent of updateCreds with flat profiles throws an error, but works fine
      • Entering credentials for the first time in a flat config file displays an error message but the operation works and the credentials are propagated across ZE
        image
    • ✅ login with base profile works fine
    • ✅ logout works fine for regular and base
    • ❓ login with regular/nested profiles
      • I noticed that the same profile in other trees does prompt for credentials to login, however, it works fine if I escape that first prompt and go to the actual search. That means that the credentials are propagated, but perhaps the checkCurrentProfile (or similar method) is not aware of the new credentials.
  • With autoStore: true:
    • Mostly the same as autoStore: false, but I ran into the pop-up frequently for the updareCreds approach
      image

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>
@t1m0thyj
Copy link
Member Author

@zFernand0 Thanks for the detailed testing and feedback, I believe issues should be fixed now 😋

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>
Copy link

sonarcloud bot commented Sep 25, 2024

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

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.

issues with autoStore: true have been resolved.

However, I feel autoStore: false isn't ideal yet 😋

  • Credentials are not propagated between trees (which I believe is expected)
  • Login is propagated (even though you have to escape the login prompt), but logout is not.
    • Logout happens to remove the credential propagation, i.e. I can't navigate inside folders, but it lets you navigate (perhaps from cached contents) to the same directory from before (even though the token should be expired)

I know autoStore: false is not the most common configuration, and the things I mentioned here are small enough that people should not even encounter unless they are purposely trying to get into a weird state. 😋

TL;DR: LGTM! 😋

@JillieBeanSim JillieBeanSim merged commit c1d216b into main Sep 25, 2024
23 of 24 checks passed
@JillieBeanSim JillieBeanSim deleted the fix/update-credentials-desync branch September 25, 2024 12:36
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.

I'm still running into a 401 error when opening PDS members - here's the scenario I tested:

  1. Enter a search pattern for one of your profiles with pre-existing, valid credentials in place
  2. With the session expanded and data sets listed, right-click on profile node and select Manage Profile -> Update Credentials
  3. Enter in invalid credentials for the profile
  4. When the "invalid credentials" prompt occurs for the profile, cancel out of the prompt
    a. Notice that the children elements for the profile node are removed
  5. Right click on profile node -> Manage Profile -> Update Credentials
  6. Enter in valid credentials this time
  7. Click on profile node to toggle its collapsible state to expanded
  8. Open one of the PDS in the list
  9. Click on one of its members

I realized this was merged in the middle of my review, but figured I should post it anyway as it will likely need addressed in a future PR. The scenario is a bit niche, but could be reproduced if a user doesn't update their invalid creds right away.

image

@JillieBeanSim
Copy link
Contributor

@t1m0thyj can a bug report be created for the edge case please? TIA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ported Apply to issues or PRs that need ported size/XL
Projects
Status: Closed
4 participants