Skip to content

feat: validate input on run#560

Merged
vladfrangu merged 8 commits into
masterfrom
chore/validate-input-on-run
Jul 1, 2024
Merged

feat: validate input on run#560
vladfrangu merged 8 commits into
masterfrom
chore/validate-input-on-run

Conversation

@vladfrangu
Copy link
Copy Markdown
Member

@vladfrangu vladfrangu requested a review from B4nan April 30, 2024 18:07
@vladfrangu vladfrangu self-assigned this Apr 30, 2024
@github-actions github-actions Bot added this to the 88th sprint - Tooling team milestone Apr 30, 2024
@github-actions github-actions Bot added the t-tooling Issues with this label are in the ownership of the tooling team. label Apr 30, 2024
Copy link
Copy Markdown
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some tests would be nice, otherwise looking good

@vladfrangu vladfrangu force-pushed the chore/validate-input-on-run branch from 87ca352 to 49da417 Compare June 21, 2024 16:01
@vladfrangu vladfrangu marked this pull request as ready for review June 21, 2024 16:02
@github-actions github-actions Bot added the tested Temporary label used only programatically for some analytics. label Jun 21, 2024
@vladfrangu vladfrangu requested a review from B4nan June 23, 2024 19:35
@vladfrangu vladfrangu merged commit fb4354d into master Jul 1, 2024
@vladfrangu vladfrangu deleted the chore/validate-input-on-run branch July 1, 2024 12:45
@jancurn
Copy link
Copy Markdown
Member

jancurn commented Jul 1, 2024

Thanks @vladfrangu

@netmilk please did you review this is consistent with the new CLI design?

Guys, please assign Adam as reviewers to all PRs related to CLI, to ensure consistency with the new design

Comment thread src/commands/run.ts
}
}

private async validateAndStoreInput(inputOverride?: Record<string, unknown>) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment would be good

Comment thread src/lib/input_schema.ts
}
};

export const getDefaultsAndPrefillsFromInputSchema = (inputSchema: any) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments please, especially for exported functions

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add these in a follow up PR

@netmilk
Copy link
Copy Markdown
Contributor

netmilk commented Jul 2, 2024

EDIT: Disregard this comment. This comment below is on a different topic. It belongs here: #577. Many apologies.

@jancurn Yes, I've validated it in the E2E product vise. The flags interface design follows the new design style guide. But I've just noticed that piping to stidn without a flag as an alias to --input-file=- seems undocumented. Is it @vladfrangu, or am I missing it somewhere?

$ jo usernames[]=netmilk | apify call netmilk/sherlock
Run: Calling Actor netmilk/sherlock (sSuwVmyjxzabXcoMh)
2024-07-01T15:26:11.619Z ACTOR: Pulling Docker image of build ASIrG38fzQ3FLo0YO from repository.
2024-07-01T15:26:26.570Z ACTOR: Creating Docker container.
2024-07-01T15:26:26.740Z ACTOR: Starting Docker container.
...
$ apify call --help
...
FLAGS
...
  -i, --input=<value>            Optional JSON input to be given to the Actor.
...
      --input-file=<value>       Optional path to a file with JSON input to be given to the Actor. The file must be a valid JSON file. You can
                                 also specify `-` to read from standard input.

However, the canonical call command itself is supposed to be under the $ apify actors namespace in the future CLI design, and the call on root is going to be an alias to that.

Note to self: That reminds me we should create a milestone and a plan for the transformation to the "new CLI design" and make the design a public, living document here in this repo.

@vladfrangu
Copy link
Copy Markdown
Member Author

This PR was before our discussion about stdin flags @netmilk, I will fix it today in a subsequent release. Also this is about the run command, not the call command

@netmilk
Copy link
Copy Markdown
Contributor

netmilk commented Jul 2, 2024

@vladfrangu @jancurn Darn, my bad. 🤦‍♂️ Silly. Apologies. I'll look into it.

@netmilk
Copy link
Copy Markdown
Contributor

netmilk commented Jul 2, 2024

@vladfrangu I need help figuring out whether this is even changing the interface. I assume it doesn't. But apart from that, it works as expected. Thank you!

Shall we consider returning the JSON path (pointer) to the INPUT data structure in the error message in case it isn't flat? It's going to be hard to figure out which part of the input is failing the validation otherwise.

$ jo foo=bar | apify run
    Error: The input in your storage is invalid. Please fix the following errors:
      - Field awesome is required
$ echo $?
1
$ apify run --input=$(jo foo=bar)
    Error: The input provided is invalid. Please fix the following errors:
      - Field awesome is required
$ jo foo=bar > invalid.json && apify run --input-file=invalid.json
    Error: The input provided is invalid. Please fix the following errors:
      - Field awesome is required
$ apify run --input-file=invalid.json 2> error | cat -
$ cat invalid.json | apify run --input-file=-
Error: Cannot read input file at path "/Users/netmilk/projects/apify-cli-validate/test-input-validation-actor/-".
  ENOENT: no such file or directory, open '/Users/netmilk/projects/apify-cli-validate/test-input-validation-actor/-'
$ echo $?
5
$ apify run --input-file=invalid.json 2> error
$ cat error
    Error: The input provided is invalid. Please fix the following errors:
      - Field awesome is required

I've noticed one minor thing, though. The --input-file=-' doesn't work the same way it does in the call` command, can you fix that for the sake of consistency, please?

@vladfrangu
Copy link
Copy Markdown
Member Author

Yep, that's the plan for today, could you maybe also make an issue for this so we have it fully tracked? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add INPUT schema checks to "apify run"

4 participants