-
Notifications
You must be signed in to change notification settings - Fork 876
feat: add YAML support to server #6934
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
Conversation
@mafredri — this is probably 1k lines excluding generated code. I've manually tested that it works, but I'll write YAML docs and restore the support links docs in a separate PR with @bpmct to review. I also agree with making OptionSet a Edit: untagged you since you're on PTO. |
cli/clibase/yaml.go
Outdated
comment := wordwrap.WrapString( | ||
fmt.Sprintf("%s (default: %s, type: %s)", opt.Description, defValue, opt.Value.Type()), | ||
80, | ||
) | ||
nameNode := yaml.Node{ | ||
Kind: yaml.ScalarNode, | ||
Value: opt.YAML, | ||
HeadComment: wordwrap.WrapString(opt.Description, 80), | ||
HeadComment: wordwrap.WrapString(comment, 80), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment
seems to be wordwrap'd twice
cli/clibase/yaml.go
Outdated
defValue = "<unset>" | ||
} | ||
comment := wordwrap.WrapString( | ||
fmt.Sprintf("%s (default: %s, type: %s)", opt.Description, defValue, opt.Value.Type()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be nicer to read as %s\n(default: %s, type: %s)
so default is always on the last line. Looking at the golden file it was hard to parse when the default/type randomly wrapped at 80 chars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big fan of including the description as a comment, and the word wrapping makes it easy to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
# An HTTP URL that is accessible by other replicas to relay DERP traffic. Required | ||
# for high availability. | ||
# (default: <unset>, type: url) | ||
relayURL: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like url types are the only fields that are printed out empty when the default is unset, the others seem to use the zero value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the type would be better as string(url)
so that it's clear you're inputting a string here? Unsure though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for the most part it's self-explanatory.
This re-releases support links, amongst other benefits to operability.
Follow-up:
[]*Option
to avoid all the complex indirection seen in this PR