-
Notifications
You must be signed in to change notification settings - Fork 4.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
[Docs] Connect Sidecar Proxies: Fixes 'must may' typo and adds formatting to follow other docs #5397
Conversation
proxy should attempt to connect to the local application instance on. | ||
Defaults to 127.0.0.1. | ||
|
||
- `local_service_port` `int: <optional>` - Specifies the port a side-car | ||
- `local_service_port` `(int: <required>)` - Specifies the port a side-car |
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 is the only one that switched from optional to required; just want to make sure it isn't a mistake.
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.
Yeah it is optional. This is different form the upstream bind port above which is required currently.
If not provided we default to the service's advertised port as noted in the comment.
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.
Ah glad you queried that @judithpatudith ! Can't think why I thought it needed changing, does this effect line 81 above also? Will look back at what I was doing also.
I think I might have thought 'this should be optional' as I don't think normal proxies require it, but sidecars do? Is that the difference here? Thats not right! Still trying to refresh memory!
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.
Cool, I've reverted that required
back to optional
here
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.
Judith pointed out the one slight issue with this but thanks for formatting updates!
I'm not quire sure what you change it to though - the prose below describes the default behaviour. Maybe it's clear enough to show it as 0 default but I think that's less clear than now although maybe more consistent?
proxy should attempt to connect to the local application instance on. | ||
Defaults to 127.0.0.1. | ||
|
||
- `local_service_port` `int: <optional>` - Specifies the port a side-car | ||
- `local_service_port` `(int: <required>)` - Specifies the port a side-car |
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.
Yeah it is optional. This is different form the upstream bind port above which is required currently.
If not provided we default to the service's advertised port as noted in the comment.
…ting to follow other docs (#5397) * Fixes 'must may' typo and adds formatting to follow other docs * Reverts local_service_port docs to state its optional
must may
typo - replaced withshould
(??).<required>
to follow the styling of other docs, removes 'optional' which we seem to just use empty strings for elsewhere.string
withint
in certain places (ports)Lastly I wasn't sure whether to remove the optional thing here, but I looked elsewhere and the only place we do that is in
operator/area
. Happy to revert that if we still want that here. I think the main thing here is themust may
typo.