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 support for a YAML config file #1619

Merged
merged 11 commits into from
May 13, 2020
Merged

Add support for a YAML config file #1619

merged 11 commits into from
May 13, 2020

Conversation

nhooyr
Copy link
Contributor

@nhooyr nhooyr commented May 10, 2020

No description provided.

@nhooyr nhooyr requested a review from code-asher as a code owner May 10, 2020 05:20
@nhooyr nhooyr force-pushed the config branch 3 times, most recently from 3087930 to 4d239eb Compare May 10, 2020 07:19
@nhooyr
Copy link
Contributor Author

nhooyr commented May 10, 2020

Last thing for v3.3.0 is just the docs on all the changes.

@code-asher Would be nice to have windows support as well, see #1397

@nhooyr nhooyr force-pushed the config branch 6 times, most recently from 08b3775 to 6dddb4c Compare May 12, 2020 01:21
@nhooyr
Copy link
Contributor Author

nhooyr commented May 12, 2020

note: the login page should indicate the config location or whether the password was grabbed via $PASSWORD.

@nhooyr nhooyr force-pushed the config branch 2 times, most recently from adf3703 to fd919de Compare May 12, 2020 06:47
throw error(`--${key} requires a value`)
}

if (option.type == OptionalString && value == "false") {
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious what the use of this is? An optional string is a string? rather than boolean so this feels incorrect to me as it prevents the string value false.

I see it used in the default config file but since omission already has this meaning it doesn't seem to gain anything? Unless it's to provide an example to the user in which case I think we could add it as a comment.

Copy link
Member

Choose a reason for hiding this comment

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

I could see it being nice to explicitly disable the cert in the configuration; we could definitely modify parse to handle this case but maybe we just don't worry about it until the cli library. No one's going to use a cert named false anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea the purpose was to disable the cert via the cli when it's set in the config.

@coder coder deleted a comment from codeclimate bot May 12, 2020
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

One comment, aside from that looks good to me!

nhooyr added 2 commits May 12, 2020 19:59
- Error out if auth is enabled but no password is passed in
- Indicate password location on login page
@nhooyr nhooyr merged commit 8626bed into master May 13, 2020
@nhooyr nhooyr deleted the config branch May 13, 2020 01:25
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