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

Some tweaks to CLI configuration #323

Merged
merged 3 commits into from
Jan 9, 2024
Merged

Some tweaks to CLI configuration #323

merged 3 commits into from
Jan 9, 2024

Conversation

ryguyk
Copy link
Contributor

@ryguyk ryguyk commented Jan 5, 2024

Description

  • Configures CLI to pass arguments through to the underlying utility.
  • Updates CLI help text to explain how to pass args through to underlying utility.
  • Displays help text of underlying utility (in addition to general env help) if -h is passed to the yarn script.

These changes allowed the removal of a few yarn scripts:

  • lint:staged:test-modified-files: no longer need this because we can pass the necessary commands through to yarn test now and we couldn't before. (Note: before the creation of this now-deleted file, we were simply running jest, but this yarn script was created so as to be able to run the env-loader before running jest. We can now accomplish both by running yarn test and passing arguments through to jest.)
  • test:watch: can now run yarn test -- --watch
  • test:update-snapshots: can now run yarn test -- -u (Note: We could keep test:watch and test:update-snapshots if we wanted that layer of abstraction, but I'm not sure it's necessary.)

Testing done

Manual testing of yarn scripts

QA steps

  • Run yarn scripts and ensure args can be passed through to underlying utility (e.g. jest, next)
  • Run yarn scripts with -h and see both general env help and underlying-utility help (e.g. yarn test -h)

- Updates CLI help text to explain how to pass args through to underlying utility.
- Displays help text of underlying utility (in addition to general env help) if -h is passed to the yarn script.
@va-cms-bot va-cms-bot temporarily deployed to Tugboat January 5, 2024 19:57 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat January 8, 2024 18:14 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat January 8, 2024 20:19 Destroyed
Copy link
Contributor

@tjheffner tjheffner left a comment

Choose a reason for hiding this comment

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

pulled and tested locally, works great. thank you!

I re-ordered some of the test: commands in package.json and updated test:watch to use the passthrough syntax.

// Return all options with proper cascading:
// 1. CLI options have highest precedence
// 2. CMS feature flags next
// 3. ENV file vars last
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment (or something to that effect) is valuable to keep even if it is technically self-documented in the code

...envVars,
...cliOptions,
}
// For now, don't fetch CMS feature flags for tests. Will fail CI.
Copy link
Contributor

Choose a reason for hiding this comment

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

not a blocker for this, just curious what failures you were seeing? is it related to fetching an env behind the VA network through GHA (gh-provided vs self-hosted runners), or something else?

@tjheffner tjheffner marked this pull request as ready for review January 9, 2024 19:44
@tjheffner tjheffner merged commit 25e8698 into main Jan 9, 2024
7 checks passed
@tjheffner tjheffner deleted the cli-tweaks branch January 9, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants