Skip to content

Add support for configuring a GoogleAdsClient using environment variables#386

Merged
jradcliff merged 5 commits intomasterfrom
env-vars-pr
Dec 11, 2020
Merged

Add support for configuring a GoogleAdsClient using environment variables#386
jradcliff merged 5 commits intomasterfrom
env-vars-pr

Conversation

@jradcliff
Copy link
Copy Markdown
Member

Also added support for configuring linkedCustomerId from a config
file, properties, or an environment variable.

…iables

Also added support for configuring `linkedCustomerId` from a config
file, properties, or an environment variable.
@jradcliff
Copy link
Copy Markdown
Member Author

Note to reviewers:

I considered multiple approaches here, but having a private GoogleCredentials.Builder was the cleanest approach I could find that satisfied all of the following requirements:

  1. Support merging values from environment variables, a config file, properties, and setter invocations.
  2. Do not make any breaking changes to the public interface of the client or its builder.
  3. Support either setting credentials explicitly via setCredentials(Credentials) or configuring credentials from a file, properties, or environment variables.
  4. Play nice with AutoValue. An unfortunate result of this requirement is that I had to mark getCredentials() on the client as @Nullable. There were alternatives to doing so, but they were all considerably less pleasant.

@jradcliff jradcliff requested review from devchas and nwbirnie December 9, 2020 19:20
Comment thread google-ads/src/main/java/com/google/ads/googleads/lib/GoogleAdsClient.java Outdated
Comment thread google-ads/src/main/java/com/google/ads/googleads/lib/GoogleAdsClient.java Outdated
@jradcliff
Copy link
Copy Markdown
Member Author

Heads up @nwbirnie and @devchas : I was able to find a way to implement "last one wins" logic for the 3rd requirement:

  1. Support either setting credentials explicitly via setCredentials(Credentials) or configuring credentials from a file, properties, or environment variables.

With the latest commit, users who previously took the following steps in either order will see the same behavior as always:

  • Call setCredentials(Credentials).
  • Call fromProperties(...).

That is, the last step they performed will configure their credentials. My prior commits on this PR would have made either sequence throw an exception, which would have been a breaking change.

Copy link
Copy Markdown
Contributor

@nwbirnie nwbirnie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round of comments - going to take another pass just now


@VisibleForTesting
Builder setEnvironment(Map<String, String> environment) {
this.environment = ImmutableMap.copyOf(environment);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need state to represent the environment? Can't we just load the environment as required?

It looks like you could use a Supplier<Map<String,String>> to represent abstract this for testing purposes.

Copy link
Copy Markdown
Contributor

@nwbirnie nwbirnie Dec 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nit.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. I swapped this out with a Function<String, String> and it worked out quite nicely + avoided copying the environment map.

@nwbirnie
Copy link
Copy Markdown
Contributor

LGTM pending open comments

Comment thread google-ads/src/main/java/com/google/ads/googleads/lib/GoogleAdsClient.java Outdated
Comment thread google-ads/src/main/java/com/google/ads/googleads/lib/GoogleAdsClient.java Outdated
@jradcliff jradcliff requested a review from nwbirnie December 10, 2020 22:51
@jradcliff
Copy link
Copy Markdown
Member Author

I merged from master at d632e3ae in preparation for merging this. Please use Jump to to look at the two files that actually changed: GoogleAdsClient.java and GoogleAdsClientTest.java.

@jradcliff jradcliff merged commit a826181 into master Dec 11, 2020
@jradcliff jradcliff deleted the env-vars-pr branch February 22, 2024 16:53
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.

3 participants