-
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
Add secrets-for-zowe-sdk dependency to ZE API #2423
Conversation
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2423 +/- ##
=======================================
Coverage 92.62% 92.62%
=======================================
Files 98 98
Lines 9411 9414 +3
Branches 1951 1951
=======================================
+ Hits 8717 8720 +3
Misses 693 693
Partials 1 1
☔ View full report in Codecov by Sentry. |
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
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.
Thanks for this change Billie!
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.
LGTM! 😋
TLDR: (if possible) I would like to see the CLI being exported from the ZE API
That's the reason for my requested changes.
We should merge this PR before Rudy's.
Definitely fixes the following issue in Rudy's PR: 😋
[2023/08/21 13:16:28.483] [ERROR] [CredentialManagerFactory.js:151] Error: Failed
to load Keytar module: Cannot find module '@zowe/secrets-for-zowe-sdk'
Also, I didn't know whether this is the right place to ask for this or not... but could we export an instance of zowe
from the zowe-explorer-api?
That way, VSCode Extenders don't have to import from node_modules of the ZE API in order to bypass the annoying ProfileInfo error not being from the same location as the ZE API.
And there is one more thing to note (for Extenders)...
There are a few methods that could be broken given that the function signature of getProfileFromConfig
changed to include the undefined
.
- How is that breaking?
- It's possible for an extenders to have defined something of type
IProfAttrs
and TypeScript won't let them transpile/compile because it's not the same as the union ofIProfAttrs | undefined
😋
- It's possible for an extenders to have defined something of type
Last, but certainly not least...
In the event that an extenders has not updated their CLI dependency, we could be breaking them by introducing new optional method to the ZE API...
- How?
- For instance the
dataSetsMatchingPattern
has a signature that includes an interface calledzowe.IDsmListOptions
, which was introduced in@zowe/cli@7.3.0
- the CICS extension still had 7.2.3 😋
- For instance the
So, you can imagine if an extender has a version of the CLI lower than that, then they may get issues like this:
node_modules/@zowe/zowe-explorer-api/lib/profiles/ZoweExplorerApi.d.ts:297:67 - error
TS2724: '"/root/gh/zowe/vscode-extension-for-cics/node_modules/@zowe/cli/lib/index"'
has no exported member named 'IDsmListOptions'. Did you mean 'IUSSListOptions'?
297 dataSetsMatchingPattern?(filter: string[], options?: zowe.IDsmListOptions):
Promise<zowe.IZosFilesResponse>;
I believe this could be prevented if the ZE API exported their version of @zowe/cli
and extenders used that one, instead having another dependency in their package.json files
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.
Just left a quick comment on how the dependencies are structured, but everything LGTM. Thanks @JillieBeanSim
I like the idea of exporting the Imperative APIs from zowe-explorer-api to avoid errors related to Perhaps we could export just the |
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
This works perfectly fine for me using CICS.
I tested this PR in combination with Rudy's:
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.
Thanks for addressing this @JillieBeanSim!
Proposed changes
After some testing of importing ZE API as a dependency for extender since the @zowe/secrets-for-zowe-sdk was added and saw issues. This PR adds the dependency to the package and moves the zowe-explorer's to top-level similar to how we handle the @zowe/cli dependency in the monorepo. I also added the check in the ZE API build command script for the package.
Release Notes
Milestone: 2.10.0
Changelog: n/a
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