needless async in ConfigLoader?

[aviramha]

while writing test for #1331 I noticed that most functions of ConfigLoader are async for no reason (no await)
I thought to send a PR changing that, but that'd be a breaking change - so I think maybe we should leave a note to change the API when releasing a major?

[clux]

hey, nice spot. I think it's a nice change, but it's also a little awkward. The diff works out like;

diff --git a/examples/pod_reflector.rs b/examples/pod_reflector.rs
index ab41f79..becc001 100644
--- a/examples/pod_reflector.rs
+++ b/examples/pod_reflector.rs
@@ -10,7 +10,7 @@ use tracing::*;
 #[tokio::main]
 async fn main() -> anyhow::Result<()> {
     tracing_subscriber::fmt::init();
-    let client = Client::try_default().await?;
+    let client = Client::try_default()?;
 
     let api: Api<Pod> = Api::default_namespaced(client);
     let (reader, writer) = reflector::store::<Pod>();
diff --git a/kube-client/src/client/mod.rs b/kube-client/src/client/mod.rs
index 83d87e7..cab3eb4 100644
--- a/kube-client/src/client/mod.rs
+++ b/kube-client/src/client/mod.rs
@@ -125,8 +125,8 @@ impl Client {
     ///
     /// If you already have a [`Config`] then use [`Client::try_from`](Self::try_from)
     /// instead.
-    pub async fn try_default() -> Result<Self> {
-        Self::try_from(Config::infer().await.map_err(Error::InferConfig)?)
+    pub fn try_default() -> Result<Self> {
+        Self::try_from(Config::infer().map_err(Error::InferConfig)?)
     }
 
     /// Get the default namespace for the client
diff --git a/kube-client/src/config/file_loader.rs b/kube-client/src/config/file_loader.rs
index 2eeb1b0..ebf0cb7 100644
--- a/kube-client/src/config/file_loader.rs
+++ b/kube-client/src/config/file_loader.rs
@@ -25,20 +25,18 @@ pub struct ConfigLoader {
 
 impl ConfigLoader {
     /// Returns a config loader based on the cluster information from the kubeconfig file.
-    pub async fn new_from_options(options: &KubeConfigOptions) -> Result<Self, KubeconfigError> {
+    pub fn new_from_options(options: &KubeConfigOptions) -> Result<Self, KubeconfigError> {
         let config = Kubeconfig::read()?;
         let loader = Self::load(
             config,
             options.context.as_ref(),
             options.cluster.as_ref(),
             options.user.as_ref(),
-        )
-        .await?;
-
+        )?;
         Ok(loader)
     }
 
-    pub async fn new_from_kubeconfig(
+    pub fn new_from_kubeconfig(
         config: Kubeconfig,
         options: &KubeConfigOptions,
     ) -> Result<Self, KubeconfigError> {
@@ -47,13 +45,11 @@ impl ConfigLoader {
             options.context.as_ref(),
             options.cluster.as_ref(),
             options.user.as_ref(),
-        )
-        .await?;
-
+        )?;
         Ok(loader)
     }
 
-    pub async fn load(
+    pub fn load(
         config: Kubeconfig,
         context: Option<&String>,
         cluster: Option<&String>,
diff --git a/kube-client/src/config/mod.rs b/kube-client/src/config/mod.rs
index c114a9f..7fc3c0d 100644
--- a/kube-client/src/config/mod.rs
+++ b/kube-client/src/config/mod.rs
@@ -186,8 +186,8 @@ impl Config {
     ///
     /// [`Config::apply_debug_overrides`] is used to augment the loaded
     /// configuration based on the environment.
-    pub async fn infer() -> Result<Self, InferConfigError> {
-        let mut config = match Self::from_kubeconfig(&KubeConfigOptions::default()).await {
+    pub fn infer() -> Result<Self, InferConfigError> {
+        let mut config = match Self::from_kubeconfig(&KubeConfigOptions::default()) {
             Err(kubeconfig_err) => {
                 tracing::trace!(
                     error = &kubeconfig_err as &dyn std::error::Error,
@@ -264,23 +264,23 @@ impl Config {
     /// This will respect the `$KUBECONFIG` evar, but otherwise default to `~/.kube/config`.
     /// You can also customize what context/cluster/user you want to use here,
     /// but it will default to the current-context.
-    pub async fn from_kubeconfig(options: &KubeConfigOptions) -> Result<Self, KubeconfigError> {
-        let loader = ConfigLoader::new_from_options(options).await?;
-        Self::new_from_loader(loader).await
+    pub fn from_kubeconfig(options: &KubeConfigOptions) -> Result<Self, KubeconfigError> {
+        let loader = ConfigLoader::new_from_options(options)?;
+        Self::new_from_loader(loader)
     }
 
     /// Create configuration from a [`Kubeconfig`] struct
     ///
     /// This bypasses kube's normal config parsing to obtain custom functionality.
-    pub async fn from_custom_kubeconfig(
+    pub fn from_custom_kubeconfig(
         kubeconfig: Kubeconfig,
         options: &KubeConfigOptions,
     ) -> Result<Self, KubeconfigError> {
-        let loader = ConfigLoader::new_from_kubeconfig(kubeconfig, options).await?;
-        Self::new_from_loader(loader).await
+        let loader = ConfigLoader::new_from_kubeconfig(kubeconfig, options)?;
+        Self::new_from_loader(loader)
     }
 
-    async fn new_from_loader(loader: ConfigLoader) -> Result<Self, KubeconfigError> {
+    fn new_from_loader(loader: ConfigLoader) -> Result<Self, KubeconfigError> {
         let cluster_url = loader
             .cluster
             .server
@@ -414,7 +414,7 @@ mod tests {
         let file = tempfile::NamedTempFile::new().expect("create config tempfile");
         std::fs::write(file.path(), cfgraw).unwrap();
         std::env::set_var("KUBECONFIG", file.path());
-        let kubeconfig = Config::infer().await.unwrap();
+        let kubeconfig = Config::infer().unwrap();
         assert_eq!(kubeconfig.cluster_url, "https://0.0.0.0:6443/");
     }
 }

meaning it actually bubbles all the way up into user space via Client::try_default (our main advertised starting point).

this clearly simplifies it, but it also locks it down as a "never async" type fn (because we don't want to break this more than this time if we do it), and there is one potential reason to keep it as async;

fs::read

a local config is fs::readwhich is currently done through std::fs so synchronously, but this will also mean sync code in a normally-async environment which is not the greatest practice.

config read is usually done at app boot so this is usually ok though.

---

after a quick think, i feel like we probably should do tokio::fs::read instead to actually use futures and not block the executor when we actually provide a fully async lib.

if we do that change most of the internal file_loader interfaces to not .await, but add an await on the function that actually reads the file. we don't then break the outer interface (Client::try_default still needs async because file loading is async).

but not 100% sure. curious to see if other people have strong opinions.

[nightkr]

+1 from me on tokio::fs::read (then again as an old Haskeller I tend to view async as our equivalent of the IO monad).

[aviramha]

I think that using tokio fs is a bit of an overkill (as it it's just sync-toasync inside) and we're probably just doing that wrapping ourselves from async flows when calling sync flows