Skip to content

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

Merged
merged 42 commits into from
Apr 7, 2023
Merged

feat: add YAML support to server #6934

merged 42 commits into from
Apr 7, 2023

Conversation

ammario
Copy link
Member

@ammario ammario commented Mar 31, 2023

This re-releases support links, amongst other benefits to operability.

  • Update cli docs generator to include YAML in table

Follow-up:

  • Restore supportLinks docs
  • Write docs on YAML
  • Convert OptionSet to []*Option to avoid all the complex indirection seen in this PR

@ammario ammario requested a review from mafredri April 6, 2023 23:27
@ammario
Copy link
Member Author

ammario commented Apr 6, 2023

@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 []*Option, which I'll do separately.

Edit: untagged you since you're on PTO.

@ammario ammario removed the request for review from mafredri April 7, 2023 16:05
Comment on lines 68 to 75
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),
Copy link
Contributor

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

defValue = "<unset>"
}
comment := wordwrap.WrapString(
fmt.Sprintf("%s (default: %s, type: %s)", opt.Description, defValue, opt.Value.Type()),
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

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:
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

@ammario ammario enabled auto-merge (squash) April 7, 2023 22:58
@ammario ammario merged commit 4b99e2d into main Apr 7, 2023
@ammario ammario deleted the yaml branch April 7, 2023 22:58
@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants