-
Notifications
You must be signed in to change notification settings - Fork 98
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
[vNext] Update SDKs and address breaking changes #2932
Conversation
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: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #2932 +/- ##
==========================================
- Coverage 92.74% 92.74% -0.01%
==========================================
Files 107 107
Lines 10889 10882 -7
Branches 2354 2369 +15
==========================================
- Hits 10099 10092 -7
Misses 788 788
Partials 2 2 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@t1m0thyj I will be opening an issue in @zowe/cli for an issue I see where the converted configuration file isn't being written (tested on mac and windows with same results) and also if the code to delete the @zowe/secure-credential-store plugin isn't going to be public imperative should handle the check and removal in it's APIs issue: zowe/zowe-cli#2170 @zFernand0 I tested the config file creation with this PR and still see that the base profile is missing |
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
@JillieBeanSim Regarding the first issue, @gejohnston is investigating it. Regarding the base profile missing, I pushed a commit that should fix this. It seems kind of redundant that the |
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.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: Timothy Johnson <timothy.johnson@broadcom.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.
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.
Looks pretty good, thanks @JillieBeanSim! Left a few comments
@@ -80,6 +81,7 @@ All notable changes to the "zowe-explorer-ftp-extension" extension will be docum | |||
### Bug fixes | |||
|
|||
- Fixed ECONNRESET error when trying to upload or create an empty data set member. [#2350](https://github.com/zowe/vscode-extension-for-zowe/issues/2350) | |||
- Update Zowe SDKs to `8.0.0-next.202407051717` for technical currency. [#2918](https://github.com/zowe/zowe-explorer-vscode/issues/2918) |
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 remove this line (seems redundant) and update line 13 above to have latest timestamp (8.0.0-next.202407051717)?
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 think we can keep the pre-release version updates in CHANGELOGs, this may help extenders know the compatible CLI/SDK dependency version per Zowe Explorer pre-release. We will create a nice clean CHANGELOG for the GA
@@ -571,4 +562,46 @@ export class ProfilesUtils { | |||
} | |||
throw new Error(vscode.l10n.t("Tree Item is not a Zowe Explorer item.")); | |||
} | |||
|
|||
private static async convertV1Profs(): Promise<void> { |
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.
Noticed the logic for building a response message after converting v1 profiles has moved from ProfilesCache in ZE API to here. Do we think that extenders who wish to convert profiles don't need this code and plan to leave it up to them to process the response themselves?
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 should keep the original response in the API call return so Extenders can manipulate the information how they wish.
I tested some of the conversion functionality on Linux, and some of the output looks a bit odd once the conversion is complete. More specifically, the output appears to be very squished together. Not asking for a change, just raising a bit of awareness in case this wasn't seen in other operating systems. |
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Pushed a commit that should make the message easier to read 😋 @JillieBeanSim @zFernand0 I also ended up changing |
Thanks for addressing the format of these messages. Curious if you had a chance to try and switch the order of these two calls in the await ProfilesUtils.initializeZoweProfiles((msg) => ZoweExplorerExtender.showZoweConfigError(msg));
await Profiles.createInstance(ZoweLogger.imperativeLogger); If the above is possible (to some degree), then I think we don't need to add this "breaking change". Note: we may have to break down the initializeZoweProfiles method because there may be things that need to occur before the creation of the ProfilesCache instance 😋 If that's the case, I think the change is small enough that it may not be worth causing much disruption in out activation phase 😋 |
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.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>
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.
changes LGTM! 😋
one small change requested just to clean some of the indentation/logic 😋
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 Billie! I left a comment about using Object.defineProperty
but nothing that must be changed for this PR.
I agree with Fernando's request to remove the nested then
in favor of using the value returned from Gui.infoMessage
. If the change marks my review as stale, I'll re-approve.
packages/zowe-explorer/__tests__/__unit__/utils/ProfilesUtils.unit.test.ts
Show resolved
Hide resolved
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Quality Gate passedIssues Measures |
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
Update Zowe SDKs to 8.0.0-next.202407051717 and address breaking changes
#2918
Release Notes
Milestone:
Changelog:
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