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

Add config file flag #717

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add config file flag #717

wants to merge 1 commit into from

Conversation

rasmus-kirk
Copy link

@rasmus-kirk rasmus-kirk commented Jan 3, 2024

Adds a config file flag to load in config parameters from a JSON file.

Description

Lets users load in command line configuration files as a JSON file using the -c or --config flags. Options compose, so options set by the command line and the config will combine. If the same option is set by both, the command line value takes precedence. Provided file must be valid JSON and have the .json file extension. Tested by running with the --config flag set to a file containing:

{
  "port": 3001
}

My use-case is that I'm using nix to configure Flood and providing the secret values for authentication as command line arguments would leak them to the nix store which means non-root users would be able to see the secrets.

  • Breaking change (changes that break backward compatibility of public API or CLI - semver MAJOR)
  • New feature (non-breaking change which adds functionality - semver MINOR)
  • Bug fix (non-breaking change which fixes an issue - semver PATCH)

Adds a config file flag to load in config parameters from a JSON file.
@trim21
Copy link
Collaborator

trim21 commented Jan 3, 2024

thanks for your contribution, I'll review and test this when I have spare time.

Copy link

codecov bot commented Jan 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (20a672a) 73.15% compared to head (17170bb) 73.13%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #717      +/-   ##
==========================================
- Coverage   73.15%   73.13%   -0.02%     
==========================================
  Files          62       62              
  Lines       11375    11375              
  Branches      959      963       +4     
==========================================
- Hits         8321     8319       -2     
- Misses       3040     3042       +2     
  Partials       14       14              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rasmus-kirk rasmus-kirk mentioned this pull request Jan 3, 2024
13 tasks
@trim21
Copy link
Collaborator

trim21 commented Jan 4, 2024

is it possible to support yaml as well?

@rasmus-kirk
Copy link
Author

is it possible to support yaml as well?

Not really, we can give it a custom parse function, but even if that's doable, the file will still have to have the .json file extension.

@trim21
Copy link
Collaborator

trim21 commented Jan 4, 2024

I saw config.txt in this PR

https://github.com/yargs/yargs/pull/321/files

@rasmus-kirk
Copy link
Author

I must be wrong then, well, in that case it should be possible given that we import a yaml parser. If you think yaml support is important I can try adding it later this month. Should we have two flags then, one for each, or what do you think?

@trim21
Copy link
Collaborator

trim21 commented Jan 4, 2024

I must be wrong then, well, in that case it should be possible given that we import a yaml parser. If you think yaml support is important I can try adding it later this month. Should we have two flags then, one for each, or what do you think?

can't parseFn parse file content by file name ext?

@rasmus-kirk
Copy link
Author

If we add a wrapper function that reads the file extension and applies the correct parsing function accordingly, then yeah I think so. Should be cleaner.

@trim21
Copy link
Collaborator

trim21 commented Apr 28, 2024

/ping

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.

2 participants