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

--help fail on config json error #2301

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
99e5630
all needed changes to meet AC besides tests
ATorrise Oct 9, 2024
6a39c50
changelog and codeql change
ATorrise Oct 9, 2024
1e84a49
still more tests to fix
ATorrise Oct 14, 2024
1d51d5d
i think i put ttests in the right place
ATorrise Oct 22, 2024
150cd03
Merge remote-tracking branch 'origin/master' into error-msg-help-version
ATorrise Oct 22, 2024
d978e0a
linting
ATorrise Oct 24, 2024
a307dd5
removing tests because tests in command processor seem to be enough?
ATorrise Oct 24, 2024
6387262
Update packages/imperative/CHANGELOG.md
ATorrise Oct 30, 2024
0b3ebc9
attempting to fix unit test
ATorrise Oct 30, 2024
82fd8f2
removing unneeded snaps
ATorrise Nov 4, 2024
1edf977
adding node 22 support and also removing private config constructor t…
ATorrise Nov 5, 2024
f8f520d
trying to fix my 2 added unit tests for terminal output given flags
ATorrise Nov 5, 2024
6545a33
Merge branch 'master' of https://github.com/zowe/zowe-cli into error-…
zFernand0 Nov 5, 2024
ab791ec
chore: cherry pick d7ea329742879ca1bca05c5fef503b1a57ec09b9
zFernand0 Nov 5, 2024
1b793ac
getting back to normal with changes
ATorrise Nov 5, 2024
128f61b
chore: fix lint warnings
zFernand0 Nov 5, 2024
b459781
update tests
zFernand0 Nov 5, 2024
0d670f7
removing unused vars
ATorrise Nov 5, 2024
db09359
Merge branch 'error-msg-help-version' of https://github.com/zowe/zowe…
ATorrise Nov 5, 2024
2a370f0
addresing timothys batch of suggestions
ATorrise Nov 6, 2024
a2e9282
correcting spy
ATorrise Nov 6, 2024
d332d6c
writing errors
ATorrise Nov 6, 2024
71b032a
Fix config errors not being ignored
t1m0thyj Nov 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions __tests__/__packages__/cli-test-utils/src/TestUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export function runCliScript(scriptPath: string, testEnvironment: ITestEnvironme
env: childEnv,
encoding: "buffer"
});
// eslint-disable-next-line @typescript-eslint/no-magic-numbers
if (process.platform === "darwin" && (response.error as ExecFileException)?.errno === -8) {
throw new Error(`The script file ${path.basename(scriptPath)} failed to execute. Check that it starts with a shebang line.`);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -329,8 +329,9 @@ describe("Upload file-to-data-set handler", () => {
statusMessage: "Uploading to data set"
}
});
expect(jsonObj).toMatchSnapshot();
expect(apiMessage).toMatchSnapshot();

expect(apiMessage).toBe("");

ATorrise marked this conversation as resolved.
Show resolved Hide resolved
expect(logMessage).toMatch(/success:.*false/);
expect(logMessage).toMatch(/from:.*test-file/);
expect(logMessage).toMatch(/file_to_upload:.*1/);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,21 +1,5 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Upload file-to-data-set handler process method should display error when upload file to data set 1`] = `
Object {
"apiResponse": Array [
Object {
"from": "test-file",
"success": false,
"to": "testing",
},
],
"commandResponse": "uploaded",
"success": false,
}
`;

exports[`Upload file-to-data-set handler process method should display error when upload file to data set 2`] = `""`;

exports[`Upload file-to-data-set handler process method should upload a file to a data set if requested 1`] = `
Object {
"apiResponse": Array [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ import { ITestEnvironment } from "../../../../../../__tests__/__src__/environmen
import { runCliScript } from "@zowe/cli-test-utils";
import { ITestPropertiesSchema } from "../../../../../../__tests__/__src__/properties/ITestPropertiesSchema";
import * as fs from "fs";
import { Session } from "@zowe/imperative";
import { GetJobs } from "@zowe/zos-jobs-for-zowe-sdk";

// Test Environment populated in the beforeAll();
let TEST_ENVIRONMENT: ITestEnvironment<ITestPropertiesSchema>;
Expand Down Expand Up @@ -49,14 +47,14 @@ describe("zos-jobs download output command", () => {
it("should download a job and wait for it to reach output status", async () => {
const response = runCliScript(__dirname + "/__scripts__/download-output/download_job_wait_for_output.sh",
TEST_ENVIRONMENT, [IEFBR14_JCL]);
expect(response.stderr.toString()).toBe("");
expect(response.status).toBe(0);
expect(response.stderr.toString()).toBe("");
expect(response.status).toBe(0);
});
it("should download a job and wait for it to reach active status", async () => {
const response = runCliScript(__dirname + "/__scripts__/download-output/download_job_wait_for_active.sh",
TEST_ENVIRONMENT, [IEFBR14_JCL]);
expect(response.stderr.toString()).toBe("");
expect(response.status).toBe(0);
expect(response.stderr.toString()).toBe("");
expect(response.status).toBe(0);
});
});
describe("output", () => {
Expand Down
4 changes: 4 additions & 0 deletions packages/imperative/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

All notable changes to the Imperative package will be documented in this file.

## Recent Changes

- BugFix: Enabled commands with either the `--help` or `--version` flags to still display their information despite any configuration file issues. [#2301](https://github.com/zowe/zowe-cli/pull/2301)

## `8.7.0`

- Enhancement: Added optional `proxy` object to ISession interface for extenders to pass a ProxyVariables object that would override the environment variables if in place. [#2330](https://github.com/zowe/zowe-cli/pull/2330)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,37 @@ const FAKE_HELP_GENERATOR: IHelpGenerator = {
const ENV_VAR_PREFIX: string = "UNIT_TEST";

describe("Command Processor", () => {
describe("Command Processor with --help and --version flags", () => {
let faultyConfigProcessor: CommandProcessor;

beforeEach(() => {
faultyConfigProcessor = new CommandProcessor({
envVariablePrefix: ENV_VAR_PREFIX,
definition: SAMPLE_COMMAND_DEFINITION,
helpGenerator: FAKE_HELP_GENERATOR,
rootCommandName: SAMPLE_ROOT_COMMAND,
commandLine: "",
promptPhrase: "dummyPrompt",
config: {
validate: () => ({valid: false}), // Simulate faulty config
} as any,
});

jest.spyOn(console, "log").mockImplementation(() => {}); // Prevent console logs in tests
});

afterEach(() => {
jest.restoreAllMocks();
});

it("should fail command execution without --help or --version if config is faulty", async () => {
const parms: any = { arguments: { _: ["some", "command"], $0: "" }, silent: true };
const response: ICommandResponse = await faultyConfigProcessor.invoke(parms);

expect(response).toBeDefined();
expect(response.success).toBe(false);
});
});
beforeEach(() => {
// Mock read stdin
jest.spyOn(SharedOptions, "readStdinIfRequested").mockResolvedValueOnce(false);
Expand All @@ -203,7 +234,6 @@ describe("Command Processor", () => {
process.stderr.write = ORIGINAL_STDERR_WRITE;
jest.restoreAllMocks();
});

it("should allow us to create an instance", () => {
let caughtError;

Expand Down
67 changes: 67 additions & 0 deletions packages/imperative/src/config/__tests__/Config.api.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { ConfigConstants } from "../src/ConfigConstants";
import { IConfig } from "../src/doc/IConfig";
import { IConfigLayer } from "../src/doc/IConfigLayer";
import { IConfigProfile } from "../src/doc/IConfigProfile";
import { ImperativeError, Logger } from "../..";

const MY_APP = "my_app";

Expand Down Expand Up @@ -264,6 +265,72 @@ describe("Config API tests", () => {
expect(readFileSpy).toHaveBeenCalledTimes(1);
expect(secureLoadSpy).toHaveBeenCalledTimes(1);
});
it("should error when ignoreErrors is implicit false", async () => {
const config = await Config.load(MY_APP);
const existsSpy = jest.spyOn(fs, "existsSync").mockReturnValueOnce(true);
const readFileSpy = jest.spyOn(fs, "readFileSync");
const secureLoadSpy = jest.spyOn(config.api.secure, "loadCached");
const jsoncParseSpy = jest.spyOn(JSONC, "parse").mockImplementationOnce(() => { throw "failed to parse"; });
let caughtError: ImperativeError = {} as any;
try {
config.api.layers.read();
} catch(err) {
caughtError = err;
}
expect(existsSpy).toHaveBeenCalledTimes(5); // Once for each config layer and one more time for read
expect(readFileSpy).toHaveBeenCalledTimes(1);
expect(secureLoadSpy).toHaveBeenCalledTimes(0);
expect(jsoncParseSpy).toHaveBeenCalledTimes(1);
expect(caughtError.message).toContain("Please check this configuration file for errors");
});

it("should error when ignoreErrors is explicit false", async () => {
const config = await Config.load(MY_APP);
const existsSpy = jest.spyOn(fs, "existsSync").mockReturnValueOnce(true);
const readFileSpy = jest.spyOn(fs, "readFileSync");
const secureLoadSpy = jest.spyOn(config.api.secure, "loadCached");
const jsoncParseSpy = jest.spyOn(JSONC, "parse").mockImplementationOnce(() => { throw "failed to parse"; });
let caughtError: ImperativeError = {} as any;
try {
config.api.layers.read({user: false, global: false, ignoreErrors: false});
} catch(err) {
caughtError = err;
}
expect(existsSpy).toHaveBeenCalledTimes(5); // Once for each config layer and one more time for read
expect(readFileSpy).toHaveBeenCalledTimes(1);
expect(secureLoadSpy).toHaveBeenCalledTimes(0);
expect(jsoncParseSpy).toHaveBeenCalledTimes(1);
expect(caughtError.message).toContain("Please check this configuration file for errors");
});

it("shouldnt error when ignoreErrors is true", async () => {
const config = await Config.load(MY_APP);
const existsSpy = jest.spyOn(fs, "existsSync").mockReturnValueOnce(true);
const readFileSpy = jest.spyOn(fs, "readFileSync");
const secureLoadSpy = jest.spyOn(config.api.secure, "loadCached");
const jsoncParseSpy = jest.spyOn(JSONC, "parse").mockImplementationOnce(() => { throw "failed to parse"; });

let logMsg: string = "Nothing logged";
jest.spyOn(Logger, "getConsoleLogger").mockImplementation(() => {
return {
error: jest.fn((errMsg) => {
logMsg = errMsg;
})
} as any;
});
let caughtError;
try {
config.api.layers.read({user: false, global: false, ignoreErrors: true});
} catch(err) {
caughtError = err;
}
expect(existsSpy).toHaveBeenCalledTimes(5); // Once for each config layer and one more time for read
expect(readFileSpy).toHaveBeenCalledTimes(1);
ATorrise marked this conversation as resolved.
Show resolved Hide resolved
expect(secureLoadSpy).toHaveBeenCalledTimes(1);
expect(jsoncParseSpy).toHaveBeenCalledTimes(1);
expect(caughtError).toBeUndefined();
expect(logMsg).toContain("Please check this configuration file for errors");
});
});
describe("write", () => {
it("should save the active config layer", async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1843,6 +1843,7 @@ describe("TeamConfig ProfileInfo tests", () => {
properties: { profiles: {}, defaults: {} },
global: true,
user: false,
ignoreErrors: false
});
expect(cfgSchemaBuildMock).toHaveBeenCalledWith([{
type: "some-type-with-source",
Expand Down
7 changes: 4 additions & 3 deletions packages/imperative/src/config/src/Config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,10 @@ export class Config {
opts = opts || {};

// Create the basic empty configuration
const myNewConfig = new Config(opts);
const myNewConfig = new Config();
myNewConfig.mApp = app;
myNewConfig.mActive = { user: false, global: false };
myNewConfig.mVault = opts.vault;
myNewConfig.mVault = opts?.vault;
myNewConfig.mSecure = {};

// Populate configuration file layers
Expand Down Expand Up @@ -179,7 +179,8 @@ export class Config {
exists: false,
properties: Config.empty(),
global: layer === Layers.GlobalUser || layer === Layers.GlobalConfig,
user: layer === Layers.ProjectUser || layer === Layers.GlobalUser
user: layer === Layers.ProjectUser || layer === Layers.GlobalUser,
ignoreErrors: opts?.ignoreErrors
});
}

Expand Down
3 changes: 2 additions & 1 deletion packages/imperative/src/config/src/__mocks__/Config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ export class Config {
defaults: {}
},
global: true,
user: false
user: false,
ignoreErrors: false
}];

return config;
Expand Down
18 changes: 12 additions & 6 deletions packages/imperative/src/config/src/api/ConfigLayers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { ConfigConstants } from "../ConfigConstants";
import { IConfigLayer } from "../doc/IConfigLayer";
import { ConfigApi } from "./ConfigApi";
import { IConfig } from "../doc/IConfig";
import { Logger } from "../../../logger";

/**
* API Class for manipulating config layers.
Expand All @@ -31,7 +32,7 @@ export class ConfigLayers extends ConfigApi {
* @param opts The user and global flags that indicate which of the four
* config files (aka layers) is to be read.
*/
public read(opts?: { user: boolean; global: boolean }) {
public read(opts?: { user: boolean; global: boolean; ignoreErrors?: boolean}) {
// Attempt to populate the layer
const layer = opts ? this.mConfig.findLayer(opts.user, opts.global) : this.mConfig.layerActive();
if (fs.existsSync(layer.path)) {
Expand All @@ -49,11 +50,16 @@ export class ConfigLayers extends ConfigApi {
layer.properties = JSONC.parse(fileContents.toString()) as any;
layer.exists = true;
} catch (e) {
throw new ImperativeError({
msg: `Error parsing JSON in the file '${layer.path}'.\n` +
`Please check this configuration file for errors.\nError details: ${e.message}\nLine ${e.line}, Column ${e.column}`,
suppressDump: true
});
const msg = `Error parsing JSON in the file '${layer.path}'.\n` +
`Please check this configuration file for errors.\nError details: ${e.message}\nLine ${e.line}, Column ${e.column}`;
if (!opts?.ignoreErrors){
throw new ImperativeError({
msg:msg,
suppressDump: true
});
} else {
Logger.getConsoleLogger().error(msg);
}
}
this.mConfig.api.secure.loadCached(opts);
} else if (layer.exists) {
Expand Down
1 change: 1 addition & 0 deletions packages/imperative/src/config/src/doc/IConfigLayer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,5 @@ export interface IConfigLayer {
properties: IConfig;
global: boolean;
user: boolean;
ignoreErrors?: boolean;
}
5 changes: 5 additions & 0 deletions packages/imperative/src/config/src/doc/IConfigOpts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@
import { IConfigVault } from "./IConfigVault";

export interface IConfigOpts {
/**
* Continue command execution despite config errors because version or help flag present
*/
ignoreErrors?: boolean;

/**
* Directory where global config files are located. Defaults to `~/.appName`.
*/
Expand Down
2 changes: 2 additions & 0 deletions packages/imperative/src/constants/src/Constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ export class Constants {
public static readonly JSON_OPTION_ALIAS = "rfj";
public static readonly HELP_OPTION = "help";
public static readonly HELP_OPTION_ALIAS = "h";
public static readonly VERSION_OPTION = "version";
public static readonly VERSION_OPTION_ALIAS = "V";
public static readonly HELP_EXAMPLES = "help-examples";
public static readonly HELP_WEB_OPTION = "help-web";
public static readonly HELP_WEB_OPTION_ALIAS = "hw";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,8 +260,9 @@ describe("Configuration Import command handler", () => {
it("should throw error when schema file is not valid JSON", async () => {
jest.spyOn(RestClient, "getExpectString").mockResolvedValueOnce("invalid JSON");
let error: any;

try {
await downloadSchema(new URL(schemaUrl), schemaDestPath);
await downloadSchema(new URL(schemaUrl), schemaDestPath); // Normal execution
} catch (err) {
error = err;
}
Expand Down
10 changes: 9 additions & 1 deletion packages/imperative/src/imperative/src/Imperative.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,13 @@ export class Imperative {
ConfigurationValidator.validate(config);
ImperativeConfig.instance.loadedConfig = config;

// Detect CLI arguments to determine if errors should be ignored
const ignoreErrors = process.argv.includes(Constants.OPT_LONG_DASH + Constants.HELP_OPTION) ||
process.argv.includes(Constants.OPT_SHORT_DASH + Constants.HELP_OPTION_ALIAS) ||
process.argv.includes(Constants.OPT_LONG_DASH + Constants.VERSION_OPTION) ||
process.argv.includes(Constants.OPT_SHORT_DASH + Constants.VERSION_OPTION_ALIAS) ||
process.argv[process.argv.length - 1] === require.resolve('@zowe/cli');

/**
* Get the command name from the package bin.
* If no command name exists, we will instead use the file name invoked
Expand Down Expand Up @@ -178,7 +185,7 @@ export class Imperative {

try {
ImperativeConfig.instance.config = await Config.load(configAppName,
{ homeDir: ImperativeConfig.instance.cliHome }
{ homeDir: ImperativeConfig.instance.cliHome, ignoreErrors }
);
} catch (err) {
delayedConfigLoadError = err;
Expand Down Expand Up @@ -226,6 +233,7 @@ export class Imperative {
ImperativeConfig.instance.config = await Config.load(configAppName,
{
homeDir: ImperativeConfig.instance.cliHome,
ignoreErrors,
noLoad: true
}
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ function formatLogLevelMsg(logTypeName: string) {
export const probTests: IProbTest[] = [
{
itemId: ItemId.NODEJS_VER,
probExpr: "semver.satisfies('{val}', '<18.x || 19.x || >=21.x')",
probMsg: "Only Node.js versions 18 and 20 are supported."
probExpr: "semver.satisfies('{val}', '<18.x || 19.x || 21.x || >=23.x')",
ATorrise marked this conversation as resolved.
Show resolved Hide resolved
probMsg: "Only Node.js versions 18, 20, and 22 are supported."
},
{
itemId: ItemId.NPM_VER,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1517,7 +1517,7 @@ describe("AbstractRestClient tests", () => {
const result = privateRestClient.buildOptions(resource, request, reqHeaders);
expect(Object.keys(result)).toContain('agent');
expect(headerSpy).toHaveBeenCalledWith([{'Proxy-Authorization': restSession.ISession.proxy.proxy_authorization}]);
})
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ describe("Proxy tests", () => {
const expected = {
proxyUrl: passedUrl,
protocol: HTTPS_PROTOCOL
}
};

beforeEach(() => {
jest.clearAllMocks();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@ export abstract class AbstractRestClient {
this.mLogger.info(`Using the following proxy setting for the request: ${proxyUrl.href}`);
if (this.session.ISession?.proxy?.proxy_authorization) {
reqHeaders.push({ 'Proxy-Authorization': this.session.ISession.proxy.proxy_authorization});
}
}
options.agent = ProxySettings.getProxyAgent(this.session.ISession);
}
}
Expand Down
Loading