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

Update proxy configuration format #144

Merged
merged 3 commits into from
Dec 2, 2020
Merged

Update proxy configuration format #144

merged 3 commits into from
Dec 2, 2020

Conversation

iffyio
Copy link
Collaborator

@iffyio iffyio commented Nov 22, 2020

Update proxy configuration format
Update tests and docs
Merge the server/client configuration example

Fixes #130

Currently contains #140 so will keep in draft until that's merged

@google-cla google-cla bot added the cla: yes label Nov 22, 2020
Update proxy configuration format
Update tests and docs
Merge the server/client configuration example

Fixes #130
@iffyio iffyio marked this pull request as ready for review November 25, 2020 17:39
Copy link
Contributor

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

Only blocking question I have is around ConnectionConfig - otherwise, looks really good!

I went back and forth on Source as a name - but I think it's actually the best name we could come up for that enum 👍

docs/proxy-configuration.md Show resolved Hide resolved
}

#[derive(Debug, Deserialize, Serialize)]
pub struct AdminAddress {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

@@ -36,6 +36,7 @@ pub mod external_doc_tests {
// it is only available using a nightly compiler.
// To run them locally run e.g `cargo +nightly test --doc`
#![doc(include = "../docs/extensions/filters/filters.md")]
#![doc(include = "../docs/extensions/filters/load_balancer.md")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

connection: &'a ConnectionConfig,
config: Option<&'a serde_yaml::Value>,
) -> CreateFilterArgs<'a> {
pub fn new(config: Option<&serde_yaml::Value>) -> CreateFilterArgs {
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea behind passing through the ConnectionConfig was so that Filters would be able to know if they are in client or server mode.

Is it worth adding that back in here (pass in the ProxyMode)? Or should we do it with whatever PR actually needs that information?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, yes it rings a bell now I remember we had some feature in mind that wanted it. We can include it back in the PR that needs it

@markmandel markmandel added the kind/cleanup Refactoring code, fixing up documentation, etc label Dec 2, 2020
Copy link
Contributor

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

🚢

@markmandel markmandel merged commit fbd4dd7 into master Dec 2, 2020
@markmandel markmandel deleted the iu/proxy-config branch December 2, 2020 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes kind/cleanup Refactoring code, fixing up documentation, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proxy config file format
2 participants