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

Load config from URL #12

Open
stevepentland opened this issue Dec 8, 2019 · 3 comments
Open

Load config from URL #12

stevepentland opened this issue Dec 8, 2019 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@stevepentland
Copy link
Owner

This would be nice, so that a file can be loaded from an online gist or the like

@stevepentland stevepentland added the enhancement New feature or request label Dec 8, 2019
@stevepentland stevepentland self-assigned this Dec 8, 2019
@nickpyren
Copy link

@stevepentland I was thinking to make this as part of read_in_config and detecting if the path provided is a url. Thoughts? Otherwise we could have a separate argument for this.

@stevepentland
Copy link
Owner Author

stevepentland commented Jan 6, 2020

@stevepentland I was thinking to make this as part of read_in_config and detecting if the path provided is a url. Thoughts? Otherwise we could have a separate argument for this.

My original thought was to have a distinct arg to denote sourcing from a URL, which would offload some logic where we don't bother to try and parse the path as a url and vice versa.

Likely something along the lines of --url/-u and making it conflict with CONFIG would work. Like this following snippet:

.arg(
    Arg::with_name("from-url")
        .long("url")
        .short("u")
        .help("Url to source a config file from")
        .takes_value(true)
        .multiple(false)
        .conflicts_with("CONFIG"),
)

The mandatory positional CONFIG can also have a .conflicts_with("from-url") added. This will override the positional required setting on it and allow the providing of --url to satisfy the argument requirements.

The other question is where to process this argument from. Should it be passed to libspinup as another config variable, or something else. I think we have 3 possible options:

  1. Add another field to the RunConfig struct
    • This is the easiest options, and no dependencies need to change
    • The contents can be loaded into memory and passed to the parser
    • reqwest is already a dependency of libspinup
  2. Download the contents, create a temp file, and pass the path to libspinup
    • This would require adding a dependency to the binary project
    • It would remove logic from the library, as there is no if/or for url/config
  3. Split a config lib out and allow it to deal with all configuration building
    • This is the most complex
    • It would need to have its own set of dependencies, which is a pro/con at the same time
    • However it could offer some nice distinction between the inner/outer methods of the configuration logic

Personally, I prefer the first option for simplicity, however at some point I would like to break out the config logic as in point 3, simply as part of drawing more boundaries in the future.

@nickpyren
Copy link

I think at this point I like option 1 the best as well. Mind if I pick up this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants