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(v3): Support logging in to multiple API ML instances #3019

Merged
merged 17 commits into from
Aug 20, 2024

Conversation

t1m0thyj
Copy link
Member

@t1m0thyj t1m0thyj commented Jul 23, 2024

Proposed changes

Resolves #2264 and ports #3002 to vNext branch

Also moves tests from the packages/zowe-explorer/__tests__/__unit__/misc folder into Profiles.unit.test.ts and ZoweDatasetNode.unit.test.ts where they seem to naturally fit

TODO:

Release Notes

Milestone: V3

Changelog: Added support for logging in to multiple API ML instances per team config file

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

The GIF below shows an example of multiple z/OSMF profiles using API ML on different LPARs in a single team config:
ze-multi-apiml

@t1m0thyj t1m0thyj linked an issue Jul 23, 2024 that may be closed by this pull request
@t1m0thyj t1m0thyj added this to the v3 pre-releases milestone Jul 23, 2024
Copy link

codecov bot commented Jul 29, 2024

Codecov Report

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

Project coverage is 92.89%. Comparing base (4a52341) to head (abaf074).
Report is 18 commits behind head on next.

Files Patch % Lines
...zowe-explorer/src/trees/dataset/ZoweDatasetNode.ts 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3019      +/-   ##
==========================================
+ Coverage   92.88%   92.89%   +0.01%     
==========================================
  Files         111      111              
  Lines       11159    11180      +21     
  Branches     2355     2449      +94     
==========================================
+ Hits        10365    10386      +21     
  Misses        792      792              
  Partials        2        2              

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

@t1m0thyj t1m0thyj marked this pull request as ready for review August 7, 2024 14:58
traeok
traeok previously requested changes Aug 7, 2024
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 so far, thanks Timothy! Had a couple questions regarding an arrow function and assignment of a parameter

@traeok traeok self-requested a review August 7, 2024 17:07
@traeok traeok dismissed their stale review August 7, 2024 17:08

Changes addressed in recent commit

zFernand0
zFernand0 previously approved these changes Aug 7, 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! 😋
Left some comments/question to help me understand a bit better 😓

traeok
traeok previously approved these changes Aug 8, 2024
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.

Changes LGTM - thanks Timothy for the enhancement!

Signed-off-by: Timothy Johnson <[email protected]>
@t1m0thyj t1m0thyj requested a review from traeok August 9, 2024 13:12
Copy link
Member

@awharn awharn left a comment

Choose a reason for hiding this comment

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

How does this handle multiple levels of nesting? If a token is stored in level 1, and you do a login using level 3, does the token remain at level 1, or get moved to level 2?

t1m0thyj and others added 2 commits August 15, 2024 23:57
Copy link

sonarqubecloud bot commented Aug 16, 2024

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

@t1m0thyj
Copy link
Member Author

How does this handle multiple levels of nesting? If a token is stored in level 1, and you do a login using level 3, does the token remain at level 1, or get moved to level 2?

Good catch - the token was getting moved to level 2. This should be fixed in 4708616 so the token now remains at its original level.

@t1m0thyj t1m0thyj requested a review from awharn August 16, 2024 13:19
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 few small questions, but nothing that should prevent this from being merged 😋

Great job! 🥳

Comment on lines +319 to +321
* Retrieves the base profile from Imperative to use for log in/out. If a
* nested profile name is specified (e.g. "lpar.zosmf"), then its parent
* profile is returned unless token is already stored in the base profile.
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 add this new behavior to the changelog as a new breaking change?

const config = mProfileInfo.getTeamConfig();
if (
profileName?.includes(".") &&
(baseProfileAttrs == null || !config.api.secure.securePropsForProfile(baseProfileAttrs.profName).includes("tokenValue"))
Copy link
Member

Choose a reason for hiding this comment

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

Question about this line here.
In order for this function to return the default base profile (old behavior), the token only needs to be in the secure array of the base profile and not actually stored in the vault. Is that assumption correct?

Follow up question, Does that means that this function will return the default base profile even if I already have a token stored in the parent profile?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good questions, sorry I didn't see this until after the PR was merged. Your assumption is correct 😁

For the 2nd question, that's right. I think the reason was for backwards compatibility - however we probably want to give precedence to whichever profile has tokenValue already defined 😋

@JillieBeanSim
Copy link
Contributor

love the addition of this feature, the support for multiple API ML instances was a big hit when revealed for the CLI v3 at SHARE and is great to have this day 1 of ZE v3 too 😄

@JillieBeanSim
Copy link
Contributor

JillieBeanSim commented Aug 20, 2024

if other comments don't stop the merge, I think we can merge now but would like a CHANGELOG entry @t1m0thyj. can you share one here and it can be copied to the pre-release PR when staging release? this way we don't have to go through the approval process again 😄

@JillieBeanSim JillieBeanSim merged commit 97b157e into next Aug 20, 2024
19 of 20 checks passed
@JillieBeanSim JillieBeanSim deleted the feat/multi-apiml-support-v3 branch August 20, 2024 14:57
@JillieBeanSim JillieBeanSim mentioned this pull request Aug 21, 2024
18 tasks
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.

Support logging in to multiple APIML instances per config file
5 participants