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

Remove the 'zowe profiles' command command handlers & APIs #2073

Merged
merged 80 commits into from
Mar 1, 2024

Conversation

gejohnston
Copy link
Member

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 the config 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.

    image

How to Test

  1. Commands should continue to run as before when no configuration exists or a team configuration exists. Commands should ignore V1 profiles, even when they are the only available configuration. In particular, if only V1 profiles exist, running a Zowe command now will prompt the user for connection information.
    > zowe files list data-set "SYS1.PARMLIB*"
    Enter the host name of your service: YourHostName
    Enter the user name for your service (will be hidden):
    Enter the password for your service (will be hidden):
    
  2. Use --show-inputs-only, config report-env, & config convert-profiles and observe that no references to V1 or V2 profiles are displayed.

Follow-ups

  • The "ProfileIO" class must be renamed to "V1ProfileConversion", and all write functions must be removed.
  • Any additional V1 logic that is no longer called should be removed.

Review Checklist
I certify that I have:

Additional Comments

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]>
Copy link

codecov bot commented Feb 27, 2024

Codecov Report

Attention: Patch coverage is 92.70833% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 90.93%. Comparing base (230203e) to head (a38c22d).

Files Patch % Lines
...perative/src/cmd/src/profiles/CliProfileManager.ts 81.57% 7 Missing ⚠️
...es/imperative/src/config/src/ProfileCredentials.ts 20.00% 4 Missing ⚠️
packages/imperative/src/config/src/ConfigUtils.ts 90.90% 1 Missing ⚠️
packages/imperative/src/config/src/ProfileInfo.ts 98.61% 1 Missing ⚠️
...c/imperative/src/config/cmd/report-env/EnvQuery.ts 83.33% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@zFernand0 zFernand0 mentioned this pull request Feb 27, 2024
4 tasks
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.

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

Comment on lines -21 to -31
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;
}
Copy link
Member

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 😋

Copy link
Member Author

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.

Comment on lines -1009 to -1012
public get usingTeamConfig(): boolean {
this.ensureReadFromDisk();
return this.mUsingTeamConfig;
}
Copy link
Member

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

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.

Copy link
Member Author

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.

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.

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.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
60.9% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@@ -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.");
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 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.

Copy link
Member Author

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 {
Copy link
Member

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.

Copy link
Member Author

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.

@gejohnston gejohnston merged commit 6ea4c11 into next Mar 1, 2024
18 of 19 checks passed
@gejohnston gejohnston deleted the rm-profile-handlers branch March 1, 2024 18:33
Copy link

github-actions bot commented Mar 4, 2024

Release succeeded for the next branch. 🎉

The following packages have been published:

Powered by Octorelease 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants