-
Notifications
You must be signed in to change notification settings - Fork 86
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
Remove the 'zowe profiles' command command handlers & APIs #2073
Conversation
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
…handlers Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
…efault, clearDefault, & ISetDefaultProfile.ts Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
packages/imperative/src/cmd/__tests__/profiles/CommandProfiles.unit.test.ts
Fixed
Show fixed
Hide fixed
.../imperative/__tests__/src/packages/cmd/__integration__/CliProfileManager.integration.test.ts
Fixed
Show fixed
Hide fixed
packages/imperative/src/cmd/__tests__/profiles/CliProfileManager.unit.test.ts
Fixed
Show fixed
Hide fixed
packages/imperative/src/cmd/__tests__/profiles/CliProfileManager.unit.test.ts
Fixed
Show fixed
Hide fixed
packages/imperative/src/cmd/__tests__/profiles/CliProfileManager.unit.test.ts
Fixed
Show fixed
Hide fixed
packages/imperative/src/cmd/__tests__/profiles/CliProfileManager.unit.test.ts
Fixed
Show fixed
Hide fixed
packages/imperative/src/cmd/__tests__/profiles/CliProfileManager.unit.test.ts
Fixed
Show fixed
Hide fixed
packages/imperative/src/cmd/__tests__/profiles/CliProfileManager.unit.test.ts
Fixed
Show fixed
Hide fixed
packages/imperative/src/cmd/__tests__/profiles/CliProfileManager.unit.test.ts
Fixed
Show fixed
Hide fixed
packages/imperative/src/cmd/__tests__/profiles/CliProfileManager.unit.test.ts
Fixed
Show fixed
Hide fixed
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #2073 +/- ##
==========================================
- Coverage 91.09% 90.93% -0.16%
==========================================
Files 638 616 -22
Lines 18840 17358 -1482
Branches 3814 3613 -201
==========================================
- Hits 17162 15785 -1377
+ Misses 1677 1572 -105
Partials 1 1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Gene Johnston <[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.
TL;DR: LGTM! 😋
I've been lucky enough to have had tested this PR thanks to the other PRs (#2067 and a little bit in #2062) that depended on this branch (rm-profile-handlers
)
I might continue checking other parts of this PR tomorrow 😋
In ZE you can find a branch called prep/rm-profile-handlers
. This should contain a "working" version of the extension with this branch linked up.
This is the draft PR in case you want to see some of the minor changes:
I ran into a runtime problem after loading ZE, however I do believe it is due to how I linked the project. For exmaple, I just had a bunch of PNPM package overrides that looked kind of like this:
- "@zowe/core-for-zowe-sdk": "file:///root/gh/zowe/zowe-cli/packages/core",
In case it rings any bells...
TL;DR: TypeError: Cannot read properties of undefined (reading 'AsymmetricMatcher')
Here is a partial stack trace of the error.
TypeError: Cannot read properties of undefined (reading 'AsymmetricMatcher')
at ../../node_modules/.pnpm/[email protected]/node_modules/jest-diff/build/index.js (/root/gh/zowe/vscode-extension-for-zowe/packages/zowe-explorer/out/src/vendors.extension.js:797:1904)
at r (/root/gh/zowe/vscode-extension-for-zowe/packages/zowe-explorer/out/src/runtime.extension.js:12:144)
at ../../node_modules/.pnpm/file+..+zowe-cli+packages+imperative/node_modules/@zowe/imperative/lib/utilities/src/diff/DiffUtils.js (/root/gh/zowe/vscode-extension-for-zowe/packages/zowe-explorer/out/src/vendors.extension.js:597:2033)
at r (/root/gh/zowe/vscode-extension-for-zowe/packages/zowe-explorer/out/src/runtime.extension.js:12:144)
at ../../node_modules/.pnpm/file+..+zowe-cli+packages+imperative/node_modules/@zowe/imperative/lib/utilities/index.js (/root/gh/zowe/vscode-extension-for-zowe/packages/zowe-explorer/out/src/vendors.extension.js:584:9595)
at r (/root/gh/zowe/vscode-extension-for-zowe/packages/zowe-explorer/out/src/runtime.extension.js:12:144)
at ../../node_modules/.pnpm/file+..+zowe-cli+packages+imperative/node_modules/@zowe/imperative/lib/cmd/src/help/abstract/AbstractHelpGenerator.js (/root/gh/zowe/vscode-extension-for-zowe/packages/zowe-explorer/out/src/vendors.extension.js:332:2099)
at r (/root/gh/zowe/vscode-extension-for-zowe/packages/zowe-explorer/out/src/runtime.extension.js:12:144)
at ../../node_modules/.pnpm/file+..+zowe-cli+packages+imperative/node_modules/@zowe/imperative/lib/cmd/index.js (/root/gh/zowe/vscode-extension-for-zowe/packages/zowe-explorer/out/src/vendors.extension.js:195:6147)
at r (/root/gh/zowe/vscode-extension-for-zowe/packages/zowe-explorer/out/src/runtime.extension.js:12:144)
If you look closer, you might see a bunch of index.js
references from the file:///root/gh/...../imperative
package
export function getZoweDir(): string { | ||
const defaultHome = path.join(os.homedir(), ".zowe"); | ||
if (ImperativeConfig.instance.loadedConfig?.defaultHome !== defaultHome) { | ||
ImperativeConfig.instance.loadedConfig = { | ||
name: "zowe", | ||
defaultHome, | ||
envVariablePrefix: "ZOWE" | ||
}; | ||
} | ||
return ImperativeConfig.instance.cliHome; | ||
} |
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.
Curious if we need to create an equivalent getZoweDir
method in the ProfileInfo APIs for Zowe Explorer
Please note that there is only one reference in Zowe Explorer to this method
public static getZoweDir(): string {
return zowe.getZoweDir();
}
So we could just move it over there 😋
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 removed ProfileUtils.ts because I found no use of any of its functions (including getZoweDir). I missed ZE's use of getZoweDir. I could resurrect ProfileUtils.ts with just getZoweDir, or place getZoweDir into profileUtils.ts as you suggested. Do you think one choice is better than the other?
Either way, I think one of those changes should be made before merging this PR into next. Thanks.
public get usingTeamConfig(): boolean { | ||
this.ensureReadFromDisk(); | ||
return this.mUsingTeamConfig; | ||
} |
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.
There were also a few references to the ProfileInfo.usingTeamConfig
- https://github.com/zowe/vscode-extension-for-zowe/blob/next/packages/zowe-explorer-api/src/profiles/ProfilesCache.ts#L153-L155
packages/zowe-explorer/src/utils/ProfilesUtils.ts(418,26)
packages/zowe-explorer/src/utils/ProfilesUtils.ts(475,26)
packages/zowe-explorer/src/utils/ProfileManagement.ts(292,22)
packages/zowe-explorer/src/Profiles.ts(889,22)
Since this is vNext, it doesn't make much sense to keep this reference.
Thus I suggest using the new ProfileInfo.onlyV1ProfilesExist
to prevent V1 operations going forward.
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.
Even though this requires changes for consumers, I agree that we should all completely stop using ProfileInfo.usingTeamConfig. It is confusing to someone reading the logic. Does it mean we should be trying to use a team config, or does it mean that we have actually found an existing team config? To reinforce this, the following text is part of our breaking changes document:
Removed:
- ProfileInfo.usingTeamConfig
- To detect if a team config exists use ProfileInfo.getTeamConfig
- To detect if only V1 profiles exist use ProfileInfo.onlyV1ProfilesExist
Finally, it is possible that some of those ZE references should be just be removed. In many cases ZE should only be trying to read a team config, so there may be no need for a test.
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 @gejohnston! 😁
I had a couple of questions regarding an integration test case and the type definition for one of the profile-related properties, but nothing that should hold up this PR.
...ve/__tests__/src/packages/profiles/__integration__/ProfileCommandExample.integration.test.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Signed-off-by: Gene Johnston <[email protected]>
Quality Gate failedFailed conditions |
@@ -312,7 +312,7 @@ export class ProfileIO { | |||
private static crashInTeamConfigMode() { | |||
if (ImperativeConfig.instance.config?.exists) { | |||
try { | |||
throw new Error("A Zowe V1 profile operation was attempted with a Zowe V2 configuration in use."); | |||
throw new Error("A Zowe V1 profile operation was attempted with a Zowe team configuration in use."); |
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 change this term to "Zowe client configuration"? Not sure if we plan to change this terminology everywhere, or just in the output of the report-env command.
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.
There is no official policy from Rose, Adam, and Ana on the terminology that we should use for our configuration.
In V2, we needed to differentiate between our old and new configuration, the terms "V1 profiles" and "Team Configuration" made sense. Since we should be removing our references to V1 profiles, I decided to use our command output (whose text is not the primary focus of policy makers) to refer to our one and only available configuration as the "Zowe client configuration". I made an exception in the convert-profiles command to differentiate that we are converting V1 profiles to something distinctively different. Thus I kept the term "Team Configuration" in some output of that particular command. If the majority of people feel that my choice of terminology is not desired, we could revise the command output across the board in our next story.
The particular case that you sited was in ProfileIO.ts. That file was not overhauled in this PR. It will be the primary focus of the next story on V1 profiles. It is possible that we may remove that exception, maybe the entire function (I do not know for sure yet). However we resolve it, this specific message should be addressed during the next V1 story.
@@ -15,8 +15,7 @@ import { ImperativeExpect } from "../../../expect"; | |||
|
|||
/** | |||
* Profiles map created by the command profile loader and passed to the handler via parameters. Handlers can | |||
* retreive loaded profiles from the map via the profile type. | |||
* @export | |||
* retrieve loaded profiles from the map via the profile type. | |||
* @class CommandProfiles | |||
*/ | |||
export class CommandProfiles { |
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.
Are the CommandProfiles
and CommandProfileLoader
classes still needed? I thought they only existed for the purpose of working with v1 profiles.
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.
It might be possible to remove them. However, CommandProfileLoader is called by the CommandProcessor. CommandProfileLoader implies that it gathers profile information from the command's definition, which might still be relevant. I was afraid to remove it. I did mark CommandProfileLoader as @internal making it possible for us to remove it in the future without breaking consumers.
CommandProfileLoader references CommandProfiles. CommandProfiles are also referenced by numerous interfaces. Some of those interfaces are outside of the imperative package, so I could not mark CommandProfiles as @internal.
I think that the removal of these two items may require a lot change and a lot time to correct. I was not convinced that the extra baggage was worth the extra time needed to remove it. In our next V1 profiles story, after completing its primary objectives, we could decide as a team if we want to take some extra time to try to remove these items during that story. I will make a note to revisit this subject at that time.
Release succeeded for the The following packages have been published:
Powered by Octorelease 🚀 |
What It Does
This PR removes the command handlers for the
zowe profiles
command group.Also:
Numerous APIs, interfaces, and constants that supported V1 profiles were also removed.
Three classes (AbstractProfileManager which was extended by BasicProfileManager, which was extended by CliProfileManager) were consolidated into the one CliProfileManager class.
The
--show-inputs-only
options and theconfig report-env
commands no longer report V1 profile information.Replaced the term "Team configuration" with "Zowe client configuration" in the
zowe config report-env
command.Changed a reference in the
config convert-profiles
command from "V2 profiles" to "TeamConfig".Removed the display of the NVM version number in the
config report-env
command. Newer versions of NVM display a GUI popup message when launched as a subprocess, which complains that NVM should be run from a terminal (version 1.1.12 displays this behavior). The popup blocks further output until the user responds to the popup. NVM is NOT used by customers frequently enough to warrant an exhaustive effort to keep that piece of information in the report. For awareness, the NVM dialog is shown below.How to Test
--show-inputs-only
,config report-env
, &config convert-profiles
and observe that no references to V1 or V2 profiles are displayed.Follow-ups
Review Checklist
I certify that I have:
Additional Comments