feat(Hub): Support custom connectors

[kylegentle]

Switch the constraints on Hub types to use public traits based on tower_service::Service, as recommended by Hyper. This enables support for custom connectors beyond hyper_rustls::HttpsConnector.

Note: This PR is dependent on the changes in dermesser/yup-oauth2#178. Before merging this, we need:

1. feat(Authenticator client): Support custom connectors dermesser/yup-oauth2#178 to be merged.
2. A new version of yup-oauth2 to be published.
3. The yup-oauth2 version to be upgraded for google-apis-rs, either in this PR or another one.

Closes #337.

[Byron]

That's impressive work, thanks a lot!

I will be watching the linked PR and see no issues in moving this PR along once 1) and 2) are completed.

[Byron]

Now that yup-oauth2 has been upgraded with tower, I performed the upgrade here and tried the following as my minimal benchmark:

make groupsmigration1-cli-cargo ARGS=check

Unfortunately it fails with this:

error[E0308]: mismatched types
   --> src/main.rs:164:52
    |
47  | impl<'n, S> Engine<'n, S>
    |          - this type parameter
...
164 |             hub: api::GroupsMigration::new(client, auth),
    |                                                    ^^^^ expected type parameter `S`, found struct `HttpsConnector`
    |
    = note: expected struct `Authenticator<S>`
               found struct `Authenticator<HttpsConnector<HttpConnector>>`

It's strangely the very same error I got before the yup-oauth2 upgrade and wonder if some adjustments might have to be done in the CLI version. Is this something you could take a look at?

Once this builds, I believe there is nothing in the way of merging.

[kylegentle]

Thanks @Byron, seems like I missed something! I'll dig into this in the next day or two.

[kylegentle]

Okay, that should fix it! I updated the CLI code to construct our InstalledFlowAuthenticator using the hyper client with custom connector. I ran make groupsmigration1-cli-cargo ARGS=check and make groupsmigration1-cargo ARGS=check locally and both look good.

I have no experience actually using the CLIs, so it's probably a good idea to try some basic CLI operations as a sanity check to make sure all is well even beyond the compiler.

[Byron]

Thanks a lot! GroupsMigration definitely works now.

The next level of testing with make youtube3-cli-cargo ARGS=check (it covers more obscure features) seems have one kind of compile error that I think is easy to fix.

error[E0308]: mismatched types
     --> /Users/byron/dev/github.com/Byron/google-apis-rs/gen/youtube3/src/api.rs:10092:41
      |
9904  | impl<'a, S> CaptionInsertCall<'a, S>
      |          - this type parameter
...
10092 |                                 client: &self.hub.client,
      |                                         ^^^^^^^^^^^^^^^^ expected struct `HttpsConnector`, found type parameter `S`
      |
      = note: expected reference `&Client<HttpsConnector<HttpConnector>>`
                 found reference `&Client<S>`

The error is in the API (so I could have caught it before), and I assume the CLI will work as the hub initialization should be the same across all APIs.

Could you take another look?

I have no experience actually using the CLIs, so it's probably a good idea to try some basic CLI operations as a sanity check to make sure all is well even beyond the compiler.

You are absolutely right, that's what should be done. Admittedly I haven't done this for years now and (have to) trust in the power of Rust: it compiles, it works 😅.

[kylegentle]

Updated! It was a simple enough fix, adding the proper type params to ResumableUploadHelper. I verified with make youtube3-cli-cargo ARGS=check, plus the two checks above for groupsmigration API and CLI.

[Byron]

Thanks a lot for the great work! It's much appreciated and keeps these crates alive and well-integrated for a while longer!

As you can see, I am upping the stakes each time I reply while running tests locally. This time I ran make cargo-cli ARGS=check to build all CLIs and APIs.

Here is where it stumbled: make appengine1-cli-cargo ARGS=check. The reason this happens is that not all APIs use all of the generators features, but all APIs and CLIs will definitely exercise all code-paths.

warning: version requirement `3.1.0+20220226` for dependency `google-appengine1` includes semver metadata which will be ignored, removing the metadata is recommended to avoid confusion
    Checking google-appengine1 v3.1.0+20220226 (/Users/byron/dev/github.com/Byron/google-apis-rs/gen/appengine1)
error[E0255]: the name `Service` is defined multiple times
    --> /Users/byron/dev/github.com/Byron/google-apis-rs/gen/appengine1/src/api.rs:1361:1
     |
15   | use tower_service::Service;
     |     ---------------------- previous import of the trait `Service` here
...
1361 | pub struct Service {
     | ^^^^^^^^^^^^^^^^^^ `Service` redefined here
     |
     = note: `Service` must be defined only once in the type namespace of this module
help: you can use `as` to change the binding name of the import
     |
15   | use tower_service::Service as OtherService;
     |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error[E0574]: expected struct, variant or union type, found trait `Service`

I think what's happening here is that no symbol of tower should be imported, the namespace/module handling of these crates is messy enough to otherwise allow conflicts between hand-created/picked imports and auto-generated types. You can use the full path to symbols you use to fix it. Thanks again.

[Byron]

Got it, this should be easy enough to fix. Would you mind sharing the remaining validation steps that you'll be running on the next iterations?

A good point! Unfortunately I made it sound like I put you on rails of some intransparent process, even though that process isn't known to me either. I already was about to merge when it sprung into my mind that I better try to check everything given the severity of these changes. In other words, building all CLIs is the last card up my sleeve. I recommend rm -Rf gen/ beforehand to be sure it actually regenerates them - the automatic change-tracking the makefile is supposed to do stopped being reliable (at least for me).

[kylegentle]

Thanks for clarifying. I realized after my hasty reply that you already specified how to build all APIs & CLIs; that seems like the best we can throw at it! 😅

I updated the hyper and tower_service imports to use the full namespace, and tested a full build with rm -rf gen/* && make cargo-cli ARGS=check.

I really appreciate your guidance in this issue and PR; you've been a tremendous help in getting me productive on the project! I'm happy to make any more tweaks and fixes that may be needed on this.

[Byron]

Thanks for clarifying. I realized after my hasty reply that you already specified how to build all APIs & CLIs; that seems like the best we can throw at it! 😅

No worries, and I have no recollection of this 😅.

I updated the hyper and tower_service imports to use the full namespace, and tested a full build with rm -rf gen/* && make cargo-cli ARGS=check.

Great, I will do the same here for good measure and merge right after. What's the action you would like me to take afterwards? I would like to avoid re-releasing everything until there is demand - if you need particular crates I can release them separately if would know about them.

I really appreciate your guidance in this issue and PR; you've been a tremendous help in getting me productive on the project! I'm happy to make any more tweaks and fixes that may be needed on this.

I am glad, thank you ☺️! By now you a probably more familiar with many parts of the codebase than I am, and I hope you can stick around if you have an own interest in using the crates. If there is anything I can do to make your stay more agreeable, just let me know. The only thing I can think of to say thanks is to invite you as collaborator, if it would help at all.

[kylegentle]

Great, I will do the same here for good measure and merge right after. What's the action you would like me to take afterwards? I would like to avoid re-releasing everything until there is demand - if you need particular crates I can release them separately if would know about them.

google-drive3 was my primary motivator. If you could release a new version, I'd be very grateful!

I am glad, thank you ☺️! By now you a probably more familiar with many parts of the codebase than I am, and I hope you can stick around if you have an own interest in using the crates. If there is anything I can do to make your stay more agreeable, just let me know. The only thing I can think of to say thanks is to invite you as collaborator, if it would help at all.

Sure, I can stick around! I don't know about being more familiar with the codebase yet... But at a minimum, it's only fair that I help with any follow-ups relating to this change. I'd be happy to join as a collaborator.

For longer-term maintenance and stability, I think the biggest thing that would help is some kind of integration testing.

Rust rules out a lot of errors with the type system, but it still seems like this change is a bit scary to release across all the crates. Can we choose one or two CLIs or APIs that exercise all the core stuff, and set them up to run against a mock server or something? I'm not too familiar with GitHub's CI capabilities yet, but hopefully something like this is achievable.

[Byron]

google-drive3 was my primary motivator. If you could release a new version, I'd be very grateful!

Thanks for letting me know - I republished the API and CLI @v4.0.

But at a minimum, it's only fair that I help with any follow-ups relating to this change. I'd be happy to join as a collaborator.

That will be much appreciated. I have invited you as collaborator. Please keep sending PRs for changes where possible so I get a chance to take a look as well.

For longer-term maintenance and stability, I think the biggest thing that would help is some kind of integration testing.

I totally agree and would appreciate your help tremendously! Truth to be told, there is integration tests already to the point where it generates the groupsmigration and youtube APIs and builds them. There is no actual testing of the API against mocks, and I'd think doing that is tough as the mocks won't ever be actual (and ever changing) google services.

However, getting CI with its basic tests back to run would be great, and the reason I deactivated it was merely that the python parts of the toolchain kept acting up as they are absolutely out of date and… python just isn't anything one wants to rely on for long-term anything.

If you think you can get CI back to what it was, it's possible to try it now that I re-activated it. You will find what's there in .github/workflows. Fixing it is less of an exercise in GitHub CI, and more in python toolchains 😅.

If this is not for you, and trust me, I fully understand, please let me know and I deactivate CI again.