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

FE-6181 validate profile when no config is found #481

Merged
merged 7 commits into from
Dec 5, 2024

Conversation

ptpaterson
Copy link
Contributor

@ptpaterson ptpaterson commented Dec 5, 2024

Ticket(s): FE-6181

Problem

You can provide a profile when there is no config file and it will behave the same as --profile default. We should error instead.

Solution

Remove the default value for profile option set in cli.mjs. The default of "default" is already set if there is a config file.

In the case where a path to a config file cannot be found, check if the profile option was specified by the caller and throw a validation error if so.

other stuff:

Removed the mutable global argvInput, which is only used by config parsing, and instead use a closure with it to create the config parser. Not strictly necessary by any means, but I have strong feelings about side effects like this, so please holler at me if the curried function appears more confusing than the exported variable.

docs

Result

What will change as a result of your pull request? Note that sometimes this section is unnecessary because it is self-explanatory based on the solution.

Testing

Describe the manual and automated tests you completed to verify the change is working as expected.

@ptpaterson ptpaterson requested a review from a team as a code owner December 5, 2024 18:16
@ptpaterson ptpaterson changed the title Fe 6181 validate profile FE-6181 validate profile when no config is found Dec 5, 2024
Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

Minor nits. I would like to include the default in the help text tho. Thanks!

src/cli.mjs Outdated Show resolved Hide resolved
src/lib/config/config.mjs Outdated Show resolved Hide resolved
test/config.mjs Outdated Show resolved Hide resolved
test/config.mjs Outdated Show resolved Hide resolved
@echo-bravo-yahoo echo-bravo-yahoo force-pushed the fe-6181-validate-profile branch from bab230e to 191c1e8 Compare December 5, 2024 21:17
@ptpaterson ptpaterson merged commit 593d754 into v3 Dec 5, 2024
4 checks passed
@ptpaterson ptpaterson deleted the fe-6181-validate-profile branch December 5, 2024 23:49
This was referenced Dec 6, 2024
@cleve-fauna cleve-fauna mentioned this pull request Dec 13, 2024
@mwilde345 mwilde345 mentioned this pull request Dec 18, 2024
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.

4 participants