Add a new optional parameter in BigTableConnection constructor for reading Service Worker JSON.

[kiv1]

This pull request introduces a new optional parameter to the BigTableConnection class, aimed at enhancing its flexibility and ease of use by allowing the option to read configuration from a Service Worker JSON file. This addition caters to the growing need for applications to dynamically configure their BigTable connections, especially in environments where configuration management is centralised or where multiple environments require different settings.

[liufuyang]

Hey @kiv1, thanks for the contribution. Before making the API slightly more complicated, would using the GOOGLE_APPLICATION_CREDENTIALS environment parameter serve your need? If you read the doc of gcp_auth, as usually how GCP auth works in other languages or toolings, that:

Reading custom service account credentials from the path pointed to by the GOOGLE_APPLICATION_CREDENTIALS environment variable.

Otherwise, as you may see, this change breaks the current API. If you can explain the reason why this is needed while the GOOGLE_APPLICATION_CREDENTIALS cannot work for you, I think we should add another new method to create the client and allow users to throw in an AuthenticationManager.

Also in that case we do not need to write those unwrap() calls as you added in the code, we push the error handling of missing files and such to the end users.

Let me know what you think.

[kiv1]

Hi @liufuyang the reason why I need this feature is because currently our use case is to target 2 separate organisation BigTable. By adding the additional parameter this allow the reading of another organisation service worker credentials. GOOGLE_APPLICATION_CREDENTIALS only allows one instance of it as it is shared through env.

Understand that it will break the current implementation, will update on my side to reflect the changes of:

• adding another new method
• removing unwrap() to push the error handling of missing files and such to the end users

[liufuyang]

@kiv1 Thanks. Perhaps I didn't make myself clear enough. I was thinking about making a new method and call it like new_with_auth_manager(..., authentication_manager: AuthenticationManager), so the user can create one and passing in whenever needed. Then the changes here will be very simple as we just need to reuse the code in the new() methods.

Let me write some code here and see if you think making sense:

// change the old new() method to something like this:
pub async fn new_with_auth_manager(
        project_id: &str,
        instance_name: &str,
        is_read_only: bool,
        channel_size: usize,
        timeout: Option<Duration>,
        authentication_manager: AuthenticationManager,
    ) -> Result<Self> {
  // here the code will be mostly the same with the old new() method, but just
  // removing the line of
  // let authentication_manager = AuthenticationManager::new().await?;
  ...
}

// then create a new new() method and internally call new_with_auth_manager()
pub async fn new(
        project_id: &str,
        instance_name: &str,
        is_read_only: bool,
        channel_size: usize,
        timeout: Option<Duration>,
    ) -> Result<Self> {
  let authentication_manager = AuthenticationManager::new().await?;
  
  new_with_auth_manager(
    project_id, instance_name, is_read_only, channel_size, timeout, authentication_manager
  )
}

Then this change will be very simple and no need to duplicate code there.

Let me know how you think :)

[kiv1]

So sorry about that! Understood and updated accordingly.

[liufuyang]

Nothing to sorry for, thanks a lot :)

[liufuyang]

Thank you. Oh by the way, remember you are welcome to write a tiny example in the example folder. Of course, it's all up to you, as I do value your time :)

[liufuyang]

I think we should move the whole inner block of the old new() method here (for example including the whole match std::env::var("BIGTABLE_EMULATOR_HOST") {...} block here), so that the emulator connect can always work no matter how one is creating a client.

To make the Git diff look smaller, I think you can just change on the exactly old line of new() to new_with_auth_manager(), then creates a small new() after the end of new_with_auth_manager().

Let me know if it makes any sense :)

[kiv1]

Is the shifting of match std::env::var("BIGTABLE_EMULATOR_HOST") {...} correct?

[liufuyang]

Yep, I think it looks good now.

[liufuyang]

I think this is not used anywhere or?

[kiv1]

Removed 👍

[liufuyang]

I think if you use some editors it is usually easy to setup some auto clean up those white spaces while saving.

Otherwise, perhaps you can try run cargo fmt to see if those can be auto corrected.

[kiv1]

Run cargo fmt and pushed

[liufuyang]

Looks great, and thanks a lot. I will merge as soon as the CI check passes :)

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 42.1%. Comparing base (5331fa5) to head (64f3cdb).
Report is 2 commits behind head on main.

Files	Patch %	Lines
bigtable_rs/src/bigtable.rs	0.0%	19 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main     #63     +/-   ##
=======================================
- Coverage   43.9%   42.1%   -1.7%     
=======================================
  Files          6       6             
  Lines        429     447     +18     
=======================================
  Hits         188     188             
- Misses       241     259     +18     

Flag	Coverage Δ
rust	42.1% <0.0%> (-1.7%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[liufuyang]

@kiv1 I did a minor fix and brought someone else's change and just released 0.2.8, you can update and try with your new_with_auth_manager now :)

By the way, I turned it into a non-async method as there is no need to mark it as async.

[kiv1]

No problems! Thank you very much 🙏