-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(client): introduce extensible YAML config #2329
Conversation
2de4bea
to
1093e52
Compare
@sbruens @jyyi1 here is an attempt to document the config format. Please let me know how I can improve it: https://github.com/Jigsaw-Code/outline-apps/blob/fortuna-go-config/client/config.md |
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.
Thanks! LGTM with some comments!
client/go/outline/method_channel.go
Outdated
firstHop = streamFirstHop | ||
} | ||
return &InvokeMethodResult{ | ||
Value: firstHop, |
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.
Shall we return an error here or just the empty result?
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.
Not having a shared first hop is not really an error. We just won't have something to show. Perhaps firsthHop should be a struct instead, and we could show both.
Without the struct, people won't be able to use separate endpoints for tcp and udp on Windows, even if they are on the same IP, because we don't have the IP returned
provider := newTestProvider() | ||
|
||
node, err := ParseConfigYAML(` | ||
$type: ss |
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.
$type
or $parser
?
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.
Settled on $type
client/go/outline/config/parse.go
Outdated
) | ||
|
||
// ConfigParserKey is the config key used to specify the parser to use to parse an object. | ||
const ConfigParserKey = "$parser" |
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.
Should this be $parser
or $type
? The document says $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.
Settled on $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 realized I need to do a few more things to this PR before submitting. I'm adding the TODO to the description
80d1206
to
e60341a
Compare
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.
This is ready for review again. I finally got the tunnel config parsing and error propagation working
client/go/outline/config/parse.go
Outdated
) | ||
|
||
// ConfigParserKey is the config key used to specify the parser to use to parse an object. | ||
const ConfigParserKey = "$parser" |
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.
Settled on $type
provider := newTestProvider() | ||
|
||
node, err := ParseConfigYAML(` | ||
$type: ss |
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.
Settled on $type
This is part 2 of #2306.
TODO:
$parser
and$type