-
Notifications
You must be signed in to change notification settings - Fork 73
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
feat: add param noSensitiveData to Plugin.load #665
Changes from 5 commits
ff9e528
9269212
877c8e7
a2ef767
94a8c9d
5a6702a
8d21fba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,7 @@ export type Metadata = { | |
|
||
type MetadataFlag = { | ||
setFromDefault?: boolean; | ||
defaultHelp?: unknown; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to retain value from defaultHelp to be used in later steps of parse/validation. |
||
} | ||
|
||
export type ListItem = [string, string | undefined] | ||
|
@@ -55,11 +56,94 @@ export type DefaultContext<T> = { | |
flags: Record<string, string>; | ||
} | ||
|
||
export type FlagDefault<T, P = CustomOptions> = T | ((context: DefaultContext<P & OptionFlag<T, P>>) => Promise<T>) | ||
export type FlagDefaultHelp<T, P = CustomOptions> = T | ((context: DefaultContext<P & OptionFlag<T, P>>) => Promise<string | undefined>) | ||
|
||
export type ArgDefault<T, P = CustomOptions> = T | ((context: DefaultContext<Arg<T, P>>) => Promise<T>) | ||
export type ArgDefaultHelp<T, P = CustomOptions> = T | ((context: DefaultContext<Arg<T, P>>) => Promise<string | undefined>) | ||
/** | ||
* Type to define a default value for a flag. | ||
* @param context The context of the flag. | ||
* @param isWritingManifest Informs the function that a manifest file is being written. | ||
* The manifest file is used to store the flag definitions, with a default value if present, for a command and is published to npm. | ||
* When a manifest file is being written, the default value may contain data that should not be included in the manifest. | ||
* The plugin developer can use isWritingManifest to determine if the default value should be omitted from the manifest. | ||
* in the function's implementation. | ||
* @example | ||
* static flags = { | ||
* foo: flags.string({ | ||
* defaultHelp: async (context, isWritingManifest) => { | ||
* if (isWritingManifest) { | ||
* return undefined | ||
* } | ||
* return 'value that is used outside a manifest' | ||
* }, | ||
* }), | ||
* } | ||
*/ | ||
export type FlagDefault<T, P = CustomOptions> = T | ((context: DefaultContext<P & OptionFlag<T, P>>, isWritingManifest?: boolean) => Promise<T>) | ||
|
||
/** | ||
* Type to define a defaultHelp value for a flag. | ||
* The defaultHelp value is used in the help output for the flag and when writing a manifest. | ||
* It is also can be used to provide a value for the flag when issuing certain error messages. | ||
* | ||
* @param context The context of the flag. | ||
* @param isWritingManifest Informs the function that a manifest file is being written. | ||
* The manifest file is used to store the flag definitions, with a default value if present via defaultHelp, for a command and is published to npm. | ||
* When a manifest file is being written, the default value may contain data that should not be included in the manifest. | ||
* The plugin developer can use isWritingManifest to determine if the defaultHelp value should be omitted from the manifest. | ||
* in the function's implementation. | ||
* @example | ||
* static flags = { | ||
* foo: flags.string({ | ||
* defaultHelp: async (context, isWritingManifest) => { | ||
* if (isWritingManifest) { | ||
* return undefined | ||
* } | ||
* return 'value that is used outside a manifest' | ||
* }, | ||
* }), | ||
* } | ||
*/ | ||
export type FlagDefaultHelp<T, P = CustomOptions> = T | ((context: DefaultContext<P & OptionFlag<T, P>>, isWritingManifest?: boolean) => Promise<string | undefined>) | ||
|
||
/** | ||
* Type to define a default value for an arg. | ||
* @param context The context of the arg. | ||
* @param isWritingManifest Informs the function that a manifest file is being written. | ||
* The manifest file is used to store the arg definitions, with a default value if present, for a command and is published to npm. | ||
* When a manifest file is being written, the default value may contain data that should not be included in the manifest. | ||
* The plugin developer can use isWritingManifest to determine if the default value should be omitted from the manifest. | ||
* in the function's implementation. | ||
* @example | ||
* public static readonly args = { | ||
* one: Args.string({ | ||
* default: async (context, isWritingManifest) => { | ||
* if (isWritingManifest) { | ||
* return undefined | ||
* } | ||
* return 'value that is used outside a manifest' | ||
* }), | ||
* }; | ||
*/ | ||
export type ArgDefault<T, P = CustomOptions> = T | ((context: DefaultContext<Arg<T, P>>, isWritingManifest?: boolean) => Promise<T>) | ||
|
||
/** | ||
* Type to define a defaultHelp value for an arg. | ||
* @param context The context of the arg. | ||
* @param isWritingManifest Informs the function that a manifest file is being written. | ||
* The manifest file is used to store the arg definitions, with a default value if present via defaultHelp, for a command and is published to npm. | ||
* When a manifest file is being written, the default value may contain data that should not be included in the manifest. | ||
* The plugin developer can use isWritingManifest to determine if the default value should be omitted from the manifest. | ||
* in the function's implementation. | ||
* @example | ||
* public static readonly args = { | ||
* one: Args.string({ | ||
* defaultHelp: async (context, isWritingManifest) => { | ||
* if (isWritingManifest) { | ||
* return undefined | ||
* } | ||
* return 'value that is used outside a manifest' | ||
* }), | ||
* }; | ||
*/ | ||
export type ArgDefaultHelp<T, P = CustomOptions> = T | ((context: DefaultContext<Arg<T, P>>, isWritingManifest?: boolean) => Promise<string | undefined>) | ||
|
||
export type FlagRelationship = string | {name: string; when: (flags: Record<string, unknown>) => Promise<boolean>}; | ||
export type Relationship = { | ||
|
@@ -208,7 +292,7 @@ export type BooleanFlag<T> = FlagProps & BooleanFlagProps & { | |
|
||
export type OptionFlagDefaults<T, P = CustomOptions, M = false> = FlagProps & OptionFlagProps & { | ||
parse: FlagParser<T, string, P> | ||
defaultHelp?: FlagDefaultHelp<T>; | ||
defaultHelp?: T | FlagDefaultHelp<T>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need to check this change. Should not be needed |
||
input: string[]; | ||
default?: M extends true ? FlagDefault<T[] | undefined, P> : FlagDefault<T | undefined, P>; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -217,6 +217,14 @@ export class Parser<T extends ParserInput, TFlags extends OutputFlags<T['flags'] | |
const flag = this.input.flags[token.flag] | ||
|
||
if (!flag) throw new CLIError(`Unexpected flag ${token.flag}`) | ||
|
||
// if flag has defaultHelp, capture its value into metadata | ||
if (Reflect.has(flag, 'defaultHelp')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. check to see if defaultHelp is present on the flag. If so capture either constant or results of function call for later use. |
||
const defaultHelpProperty = Reflect.get(flag, 'defaultHelp') | ||
const defaultHelp = (typeof defaultHelpProperty === 'function' ? await defaultHelpProperty({options: flag, flags, ...this.context}) : defaultHelpProperty) | ||
this.metaData.flags[token.flag] = {...this.metaData.flags[token.flag], defaultHelp} | ||
} | ||
|
||
if (flag.type === 'boolean') { | ||
if (token.input === `--no-${flag.name}`) { | ||
flags[token.flag] = false | ||
|
@@ -256,7 +264,7 @@ export class Parser<T extends ParserInput, TFlags extends OutputFlags<T['flags'] | |
for (const k of Object.keys(this.input.flags)) { | ||
const flag = this.input.flags[k] | ||
if (flags[k]) continue | ||
if (flag.env && Object.prototype.hasOwnProperty.call(process.env, flag.env)) { | ||
if (flag.env && Reflect.has(process.env, flag.env)) { | ||
const input = process.env[flag.env] | ||
if (flag.type === 'option') { | ||
if (input) { | ||
|
@@ -271,7 +279,7 @@ export class Parser<T extends ParserInput, TFlags extends OutputFlags<T['flags'] | |
} | ||
|
||
if (!(k in flags) && flag.default !== undefined) { | ||
this.metaData.flags[k] = {setFromDefault: true} | ||
this.metaData.flags[k] = {...this.metaData.flags[k], setFromDefault: true} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make sure all metadata captured are retained |
||
const defaultValue = (typeof flag.default === 'function' ? await flag.default({options: flag, flags, ...this.context}) : flag.default) | ||
flags[k] = defaultValue | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -118,7 +118,8 @@ export async function validate(parse: { | |
if (parse.output.metadata.flags && parse.output.metadata.flags[name]?.setFromDefault) | ||
continue | ||
if (parse.output.flags[flag] !== undefined) { | ||
return {...base, status: 'failed', reason: `--${flag}=${parse.output.flags[flag]} cannot also be provided when using --${name}`} | ||
const flagValue = parse.output.metadata.flags?.[flag]?.defaultHelp ?? parse.output.flags[flag] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exclusive use error to be created here. To avoid |
||
return {...base, status: 'failed', reason: `--${flag}=${flagValue} cannot also be provided when using --${name}`} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,7 +47,7 @@ export class TestHelp extends Help { | |
|
||
export const commandHelp = (command?: any) => ({ | ||
async run(ctx: {help: TestHelp; commandHelp: string; expectation: string}) { | ||
const cached = await toCached(command!, {} as any) | ||
const cached = await toCached(command!, {} as any, false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Command help is being called, do not suppress sensitive data |
||
const help = ctx.help.formatCommand(cached) | ||
if (process.env.TEST_OUTPUT === '1') { | ||
console.log(help) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -684,6 +684,10 @@ See more help with --help`) | |
constructor(input: string) { | ||
this.prop = input | ||
} | ||
|
||
public toString(): string { | ||
return this.prop | ||
} | ||
} | ||
it('uses default via value', async () => { | ||
const out = await parse([], { | ||
|
@@ -707,6 +711,43 @@ See more help with --help`) | |
}) | ||
expect(out.flags.foo?.prop).to.equal('baz') | ||
}) | ||
it('should error with exclusive flag violation', async () => { | ||
try { | ||
const out = await parse(['--foo', 'baz', '--bar'], { | ||
flags: { | ||
foo: Flags.custom<TestClass>({ | ||
parse: async input => new TestClass(input), | ||
defaultHelp: new TestClass('bar'), | ||
})(), | ||
bar: Flags.boolean({ | ||
exclusive: ['foo'], | ||
}), | ||
}, | ||
}) | ||
expect.fail(`Should have thrown an error ${JSON.stringify(out)}`) | ||
} catch (error) { | ||
assert(error instanceof Error) | ||
expect(error.message).to.include('--foo=bar cannot also be provided when using --bar') | ||
} | ||
}) | ||
it('should error with exclusive flag violation and defaultHelp value', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice! |
||
try { | ||
const out = await parse(['--foo', 'baz', '--bar'], { | ||
flags: { | ||
foo: Flags.custom<TestClass>({ | ||
parse: async input => new TestClass(input), | ||
})(), | ||
bar: Flags.boolean({ | ||
exclusive: ['foo'], | ||
}), | ||
}, | ||
}) | ||
expect.fail(`Should have thrown an error ${JSON.stringify(out)}`) | ||
} catch (error) { | ||
assert(error instanceof Error) | ||
expect(error.message).to.include('--foo=baz cannot also be provided when using --bar') | ||
} | ||
}) | ||
it('uses parser when value provided', async () => { | ||
const out = await parse(['--foo=bar'], { | ||
flags: { | ||
|
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.
Have you considered adding
isWritingManifest
as a new field in theoptions
object here?https://github.com/oclif/core/blob/main/src/config/plugin.ts#L137
That way we could load plugins in
oclif manifest
like this:instead of adding a new arg to
Plugin.load