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

add moving with_default_namespace Client setter - fixes #543 #544

Merged
merged 5 commits into from Jun 7, 2021

Conversation

clux
Copy link
Member

@clux clux commented Jun 6, 2021

Follow up to #534 after custom client support in #540.

  • add moving Client::with_default_namespace setter
  • require default_namespace as an Into<String> arg in Client::new
  • update custom_client* examples to use default namespaces on pods (which are more likely to exist in default)
  • rename Config::default_ns to Config::default_namespace
  • remove Client::new_with_default_namespace alloc)

@clux clux linked an issue Jun 6, 2021 that may be closed by this pull request
@clux clux marked this pull request as ready for review June 6, 2021 09:23
@clux clux changed the title add moving with_default_ns Client setter - fixes #543 add moving with_default_namespace Client setter - fixes #543 Jun 6, 2021
@kazk
Copy link
Member

kazk commented Jun 7, 2021

Yeah, I forgot about this. with_default_namespace setter works, but still allows users to create custom client with incorrect default namespace accidentally. We can prevent that if we make default namespace required for custom client.

I don't think this is bad:

let config = Config::infer().await?;
let https = config.native_tls_https_connector()?;
let client = Client::new(
    ServiceBuilder::new()
        .layer(config.base_uri_layer())
        .option_layer(config.auth_layer()?)
        .service(hyper::Client::builder().build(https)),
    config.default_namespace,
);

It's a breaking change, but this is not widely used yet and the fix is trivial.

@clux
Copy link
Member Author

clux commented Jun 7, 2021

with_default_namespace setter works, but still allows users to create custom client with incorrect default namespace accidentally.

Yeah, that is true. But it's also possible that the user is just puts "default" there anyway. We don't really have full safety here either way. If we had more properties required (like extensions), we could have put Config as the argument:

let client = Client::new(service).with_defaults(config);
let client = Client::new(service, config);

but right now that's even more wasteful allocation-wise.

Probably thinking about this for more than I should. The explicit Into<String> arg is probably likely to catch most errors, while being the least bad. It's not like we could do impl From<Service> for Client where ALL_THE_CONSTRAITS in a nice way anyway.

I'll tweak the PR tomorrownow.

@kazk
Copy link
Member

kazk commented Jun 7, 2021

But it's also possible that the user is just puts "default" there anyway. We don't really have full safety here either way.

Yeah, my point was "forgetting" about this setter. If the default namespace was a parameter, then it's impossible to forget. I'm hoping Service stack will be flexible enough, and we don't need to add any more parameters.

@clux
Copy link
Member Author

clux commented Jun 7, 2021

Yeah, I think that's the right trade-off as well now. PR updated.
(See there's a bunch of unrelated new clippy things, but will do those separately)

kube/src/client/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: kazk <kazk.dev@gmail.com>
@clux clux merged commit 79e7e10 into master Jun 7, 2021
@clux clux deleted the default-ns-custom branch June 7, 2021 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

custom clients do not respect Api::default_namespaced
2 participants