Skip to content
This repository has been archived by the owner on Jun 28, 2022. It is now read-only.

clientconfig generates json files at toplevel #126

Closed
jmuk opened this issue May 18, 2016 · 9 comments
Closed

clientconfig generates json files at toplevel #126

jmuk opened this issue May 18, 2016 · 9 comments
Assignees
Labels
🚨 This issue needs some love. triage me I really want to be triaged.

Comments

@jmuk
Copy link
Contributor

jmuk commented May 18, 2016

When I built a gapic client for logging/v2 API, I've noticed that the generated place for clientconfig is different from the code location. client config json files are always generated at the toplevel directory, while py files are in google/logging/v2 (rb files are lib/google/logging/v2).

But our generated code expects the JSON files in the directory where the code files reside.

This means client-config target directory has to be injected by some language-specific parameters -- I don't have concrete ideas to achieve this.

@jmuk
Copy link
Contributor Author

jmuk commented May 18, 2016

@geigerj
Copy link
Contributor

geigerj commented May 18, 2016

The toolkit currently drops the client config at the top level without trying to read in any configuration (see here).

Where do the JSON client config-using languages expect the file to be located? If it can be derived from the package name, that would be an easy fix. Otherwise, let's add a field to LanguageSettingsProto in config.proto to specify the client config path.

@geigerj geigerj self-assigned this May 25, 2016
@geigerj
Copy link
Contributor

geigerj commented May 25, 2016

It looks like PHP expects the config to be located under a directory called "resources". Therefore, let's make the location optionally configurable in the GAPIC yaml. The code generator call fall back to using the package name if the JSON location is unspecified.

@geigerj
Copy link
Contributor

geigerj commented May 25, 2016

Actually, I'm now realizing that there is language-specific logic in converting a package name to a directory structure, so it's not possible to fall back to package name in a language-independent way by default.

At the same time, specifying the full path to the client config in the GAPIC yaml would have a lot of redundancy with the package name.

@jmuk @garrettjonesgoogle thoughts?

@garrettjonesgoogle
Copy link
Member

We could treat this the same way we treat the paths for the api wrapper classes - each language might or might not base it on the package name, and has a way of translating the package name to the path that it wants. So, maybe each language needs a context function to provide the right path.

@geigerj
Copy link
Contributor

geigerj commented May 25, 2016

To clarify, are you suggesting that each language would use a different provider for running the JSON client config? Right now, there's only a single, language-independent context for the client config.

@garrettjonesgoogle
Copy link
Member

I was kind of suggesting that, but that is a mistake. We just need this particular value parameterized per language somehow, but using code that transforms the existing package config value we have already into the right path for the client config for the language.

@geigerj
Copy link
Contributor

geigerj commented May 25, 2016

OK, that seems possible. We can make ClientConfigGapicProvider abstract, with output() depending on the abstract method String pathToConfig(String packageName). Each language can have a provider that extends ClientConfigGapicProvider and implements this method.

@garrettjonesgoogle
Copy link
Member

I'd prefer to use the Strategy pattern over the Template Method pattern, it makes the code a lot more modular.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🚨 This issue needs some love. triage me I really want to be triaged.
Projects
None yet
Development

No branches or pull requests

4 participants