feat: add param noSensitiveData to Plugin.load #665

peternhale · 2023-03-20T19:35:46Z

@W-12513575@

peternhale · 2023-03-20T19:45:04Z

src/config/config.ts

@@ -813,11 +813,11 @@ export class Config implements IConfig {
 }

 // when no manifest exists, the default is calculated.  This may throw, so we need to catch it
-const defaultFlagToCached = async (flag: CompletableOptionFlag<any>) => {
+const defaultFlagToCached = async (flag: CompletableOptionFlag<any>, noSensitiveData = false) => {


add noSensitiveData param to defaultFlagToCached. Value is pass to defaultHelp and default

peternhale · 2023-03-20T19:45:36Z

src/config/config.ts

    } catch {}
  } else {
    return flag.default
  }
 }

-const defaultArgToCached = async (arg: Arg<any>): Promise<any> => {
+const defaultArgToCached = async (arg: Arg<any>, noSensitiveData = false): Promise<any> => {


add noSensitiveData param to defaultArgToCached. Value is pass to defaultHelp and default

peternhale · 2023-03-20T19:46:36Z

src/config/config.ts

    } catch {}
  } else {
    return arg.default
  }
 }

-export async function toCached(c: Command.Class, plugin?: IPlugin): Promise<Command.Cached> {
+export async function toCached(c: Command.Class, plugin?: IPlugin | undefined, noSensitiveData?: boolean): Promise<Command.Cached> {


add noSensitiveData param to toCached. Value will be passes to defaultFlagToCached and defaultArgToCached functions

peternhale · 2023-03-20T19:47:08Z

src/config/plugin.ts

@@ -136,7 +136,7 @@ export class Plugin implements IPlugin {

  constructor(public options: PluginOptions) {}

-  public async load(): Promise<void> {
+  public async load(noSensitiveData?: boolean): Promise<void> {


add noSensitiveData param to load. Value is passed to _manifest

peternhale · 2023-03-20T19:47:45Z

src/config/plugin.ts

@@ -279,7 +279,7 @@ export class Plugin implements IPlugin {
      version: this.version,
      commands: (await Promise.all(this.commandIDs.map(async id => {
        try {
-          return [id, await toCached(await this.findCommand(id, {must: true}), this)]
+          return [id, await toCached(await this.findCommand(id, {must: true}), this, noSensitiveData)]


Pass noSensitiveData to toCached

peternhale · 2023-03-20T19:48:20Z

src/interfaces/parser.ts

@@ -55,11 +55,11 @@ 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 FlagDefault<T, P = CustomOptions> = T | ((context: DefaultContext<P & OptionFlag<T, P>>, noSensitiveData?: boolean) => Promise<T>)


Add noSensitiveData to flag and arg default function types

peternhale · 2023-03-20T19:49:05Z

test/help/help-test-utils.ts

@@ -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)


Command help is being called, do not suppress sensitive data

peternhale · 2023-03-20T19:51:46Z

@mshanemc With this change to core, oclif manifest will be changed to pass true to plugin.load(). Any custom default or defaultHelp functions will then be able to omit sensitive info from return values.

mshanemc

So we pass it around like a hot potato to make sure the various "consumers" get the correct value, but it's up to the user-defined default function to decide what to do.

I don't love the name...would I set it to true if I'm including sensitive information that I want omitted (and therefore containsSensitiveInformation:true would be a better name.

Or...does noSensitiveData : true mean "this contains no sensitive data" and so things should proceed as normal?

peternhale · 2023-03-20T20:24:11Z

So we pass it around like a hot potato to make sure the various "consumers" get the correct value, but it's up to the user-defined default function to decide what to do.

I don't love the name...would I set it to true if I'm including sensitive information that I want omitted (and therefore containsSensitiveInformation:true would be a better name.

Or...does noSensitiveData : true mean "this contains no sensitive data" and so things should proceed as normal?

Backwards compatibility. Plugin.load is currently a no parameter function. I could change the name to suppressSensitiveData with default of false.

@W-12513575@ @W-11868418@

peternhale · 2023-03-21T16:35:36Z

src/interfaces/parser.ts

@@ -43,6 +43,7 @@ export type Metadata = {

 type MetadataFlag = {
  setFromDefault?: boolean;
+  defaultHelp?: unknown;


Need to retain value from defaultHelp to be used in later steps of parse/validation.

peternhale · 2023-03-21T16:36:36Z

src/interfaces/parser.ts

@@ -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>;


I need to check this change. Should not be needed

peternhale · 2023-03-21T16:37:57Z

src/parser/parse.ts

@@ -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')) {


check to see if defaultHelp is present on the flag. If so capture either constant or results of function call for later use.

peternhale · 2023-03-21T16:38:45Z

src/parser/parse.ts

@@ -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}


make sure all metadata captured are retained

peternhale · 2023-03-21T16:40:43Z

src/parser/validate.ts

@@ -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]


Exclusive use error to be created here. To avoid --target-org=[object Object] cannot also be provided when using --all, see if defaultHelp value is present and if so use that in message.

mshanemc · 2023-03-22T14:30:01Z

test/parser/parse.test.ts

+          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 () => {


mshanemc

approved with non-blocking comments.

Can you add some ideas for the best way to real-world QA this, since oclif changes don't have NUT coverage?

peternhale · 2023-03-22T15:16:41Z

approved with non-blocking comments.

Can you add some ideas for the best way to real-world QA this, since oclif changes don't have NUT coverage?
@mshanemc
I am going to put this change to work in sf-plugins-core, org flags straight away. The other choice might be to create a canary plugin project in salesforcecli, so we might test plugin tooling outside the context of dx plugins.

cristiand391 · 2023-03-22T15:20:04Z

src/config/plugin.ts

+   * default is false to maintain backwards compatibility
+   * @returns Promise<void>
+   */
+  public async load(isWritingManifest?: boolean): Promise<void> {


Have you considered adding isWritingManifest as a new field in the options 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:

const plugin = new Plugin({root: fullPath, isWritingManifest: true}) await plugin.load()

instead of adding a new arg to Plugin.load

This reverts commit b4a738e.

peternhale added 2 commits March 20, 2023 13:31

feat: add new param noSensitiveData to Plugin.load

ff9e528

@W-12513575@

chore: add param to plugin interface

9269212

peternhale requested a review from mshanemc March 20, 2023 19:35

chore: add param to plugin interface

877c8e7

peternhale commented Mar 20, 2023

View reviewed changes

mshanemc requested changes Mar 20, 2023

View reviewed changes

chore: address review concerns

a2ef767

peternhale requested a review from mshanemc March 20, 2023 20:31

fix: add plugin load param that indicates menifest write underway

94a8c9d

@W-12513575@ @W-11868418@

peternhale requested a review from cristiand391 March 21, 2023 16:33

peternhale commented Mar 21, 2023

View reviewed changes

peternhale and others added 2 commits March 21, 2023 10:46

chore: remove redundant T from type

5a6702a

Merge branch 'main' into phale/no-sensitive-data

8d21fba

mshanemc reviewed Mar 22, 2023

View reviewed changes

mshanemc approved these changes Mar 22, 2023

View reviewed changes

cristiand391 approved these changes Mar 22, 2023

View reviewed changes

peternhale merged commit b4a738e into main Mar 22, 2023

peternhale deleted the phale/no-sensitive-data branch March 22, 2023 15:19

cristiand391 reviewed Mar 22, 2023

View reviewed changes

peternhale added a commit that referenced this pull request Mar 22, 2023

Revert "feat: add param noSensitiveData to Plugin.load (#665)"

b7f9d65

This reverts commit b4a738e.

peternhale added a commit that referenced this pull request Mar 22, 2023

Revert "feat: add param noSensitiveData to Plugin.load (#665)" (#670)

bdcbc87

This reverts commit b4a738e.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add param noSensitiveData to Plugin.load #665

feat: add param noSensitiveData to Plugin.load #665

peternhale commented Mar 20, 2023

peternhale Mar 20, 2023

peternhale Mar 20, 2023

peternhale Mar 20, 2023

peternhale Mar 20, 2023

peternhale Mar 20, 2023

peternhale Mar 20, 2023

peternhale Mar 20, 2023

peternhale commented Mar 20, 2023

mshanemc left a comment

peternhale commented Mar 20, 2023

peternhale Mar 21, 2023

peternhale Mar 21, 2023

peternhale Mar 21, 2023

peternhale Mar 21, 2023

peternhale Mar 21, 2023

mshanemc Mar 22, 2023

mshanemc left a comment

peternhale commented Mar 22, 2023

cristiand391 Mar 22, 2023

feat: add param noSensitiveData to Plugin.load #665

feat: add param noSensitiveData to Plugin.load #665

Conversation

peternhale commented Mar 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peternhale commented Mar 20, 2023

mshanemc left a comment

Choose a reason for hiding this comment

peternhale commented Mar 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshanemc left a comment

Choose a reason for hiding this comment

peternhale commented Mar 22, 2023

Choose a reason for hiding this comment