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

core(cli): accept flags from path #9109

Merged
merged 3 commits into from
Jun 4, 2019
Merged

Conversation

patrickhulce
Copy link
Collaborator

Summary
In order to easily accept an LH settings object over in Lighthouse CI, we need a way to easily passthrough the object. This can be done with config today but you miss out on some nice features like being able to use --budgets-path, set the --output-path, etc.

This is a low-cost way to enable consumers of the LH CLI that run in a child process programmatic access to all our CLI flags without manually (and error-pronely) constructing an argument list.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

neat

@@ -62,6 +62,9 @@ function getFlags(manualArgv) {
'lighthouse <url> --only-categories=performance,pwa',
'Only run specific categories.')

// Accept a file for all of these flags.
.config('cli-settings-path')
Copy link
Member

Choose a reason for hiding this comment

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

can you add a description specifying that other flags will override this one? (e.g. --throttling-method=provided on the command line will override "throttling-method": "devtools" in the settings file)

Copy link
Member

Choose a reason for hiding this comment

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

also, bikeshed time: should we call it something like cli-flags-path?

Copy link
Member

Choose a reason for hiding this comment

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

also, copy over to readme :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can you add a description specifying that other flags will override this one?

Yes, test added too! 👍

cli-flags-path

WFM!

also, copy over to readme

This feels close to an autopopulate script too if only we made changes more frequently 🤔

Copy link
Member

@brendankenny brendankenny Jun 4, 2019

Choose a reason for hiding this comment

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

This feels close to an autopopulate script too if only we made changes more frequently 🤔

definitely, and it's a little silly we have it at all, but I know I've at least found it useful in the past, so.... 🤷‍♀

@paulirish
Copy link
Member

wfm! 👍

@paulirish paulirish removed their request for review June 4, 2019 15:45
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM

I don't entirely understand how you're going to use it (is CI going to replicate some of the stuff in bin.js e.g. for bundget-path and output-path? or is it going to be calling the CLI?), but it makes sense on its own, too, so SG :)

@brendankenny brendankenny removed their assignment Jun 4, 2019
@patrickhulce
Copy link
Collaborator Author

patrickhulce commented Jun 4, 2019

I don't entirely understand how you're going to use it

Basically I want to use the existing logic for outputPath so I can't use the node integration, but I also want to pass a budget directly as JSON, which I can't really do from CLI flags :)

The solution that works today is a blend of config and flags but it's just so much cleaner to have it all co-located

@patrickhulce patrickhulce merged commit 7deb5ab into master Jun 4, 2019
@patrickhulce patrickhulce deleted the accept_cli_settings_path branch June 4, 2019 19:53
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.

4 participants