-
Notifications
You must be signed in to change notification settings - Fork 94
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 credential management flow #2492
Conversation
Signed-off-by: Rudy Flores <[email protected]>
Signed-off-by: Rudy Flores <[email protected]>
Signed-off-by: Rudy Flores <[email protected]>
Signed-off-by: Rudy Flores <[email protected]>
Codecov ReportAttention:
📢 Thoughts on this report? Let us know!. |
Signed-off-by: Rudy Flores <[email protected]>
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 have a quick question about this work and it's location. Should this work and these checks be in the ZE API package making it reusable for us as well as extenders? I ask this because when thinking of extenders like the CICS extension, they may need to make considerations like this if handling profiles. Right now they possibly call the ProfilesCache.getProfileInfo() that makes call to credential manager which is different than what we have here.
That's a fair point, I wouldn't be opposed to doing this change, the only thing we should confirm is the reason for having these function in |
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.
That's a fair point, I wouldn't be opposed to doing this change, the only thing we should confirm is the reason for having these function in ProfileUtils.ts to begin with,
zFernand0
t1m0thyj
any idea why we made this code in that location vs having it as code in the API originally?
I believe the reason for having this logic separated out into its own class with public static methods was that we possibly thought this could make our lives easier when maintaining this functionality (and also for making it available to extenders)
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.
see comment
Signed-off-by: Rudy Flores <[email protected]>
@rudyflores can you check the failing Theia tests please, it seems to fail consistently |
Signed-off-by: Rudy Flores <[email protected]>
Signed-off-by: Rudy Flores <[email protected]>
Signed-off-by: Rudy Flores <[email protected]>
Signed-off-by: Rudy Flores <[email protected]>
Signed-off-by: Rudy Flores <[email protected]>
Signed-off-by: Billie Simmons <[email protected]>
Signed-off-by: Billie Simmons <[email protected]>
Signed-off-by: Rudy Flores <[email protected]>
Signed-off-by: Billie Simmons <[email protected]>
Signed-off-by: Rudy Flores <[email protected]>
Signed-off-by: Rudy Flores <[email protected]>
Remove extra call and register before for theia = false
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 did some testing and was able to reproduce both dialogs. Dialog options work as expected. Thanks @rudyflores!
I noticed one minor issue with the localized strings - looks like vscode-nls
interpreted new lines from a couple of the strings and added it to the text. Other than that, LGTM
packages/zowe-explorer/i18n/sample/src/utils/ProfilesUtils.i18n.json
Outdated
Show resolved
Hide resolved
Signed-off-by: Rudy Flores <[email protected]>
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.
Good catch by Trae on the strings, I honestly didn't notice that.
- One slightly annoying thing is that you have to close/re-open (or
> Developer: Reload Window
) after selecting theUse Default
button 😋- It's possible that the above behavior is because I'm in a remote ssh env 😋
Signed-off-by: Rudy Flores <[email protected]>
Signed-off-by: Rudy Flores <[email protected]>
SonarCloud Quality Gate failed. 0 Bugs No Coverage information Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
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 @rudyflores
Proposed changes
Solves #2381
This is a restructure of our existing
ProfilesUtils.ts
static class, it will now cover the following cases we meant to address when integrating custom credential managers, this were cases such as:Having
.vsix
of credential manager installed andimperative.json
override not updated will now prompt the user to either change or stick to default@zowe/cli
in their override settingHaving
imperative.json
override updated (Installed plugin from the CLI) and.vsix
not installed will now prompt to install.vsix
if missingNOTE
This applies for any custom credential manager that is registered with imperative, not only the secrets for kubernetes plugin.
Release Notes
Milestone: v2.12.0
Changelog: Added better way of handling credential management by installing/changing missing VSIX or override setting on
imperative.json
.Types of changes
What types of changes does your code introduce to Zowe Explorer?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the revieweryarn workspace vscode-extension-for-zowe vscode:prepublish
has been executedFurther comments