Skip to content

Conversation

@timyates
Copy link
Contributor

@timyates timyates commented Jun 27, 2024

Before this PR;

  1. We had non-idiomatic field names in GoogleServiceConfiguration
  2. We were building JSON via toString()

When you create a service account with Google, they supply you with a JSON document which contains all the fields previously in GoogleServiceConfiguration.

Instead of requiring these to be copied out into separate fields for us to then re-create the JSON document on startup, this PR;

  1. Switches to a single property. This is a Base64 encoded JSON document (as supplied by Google)
  2. Adds a validator to ensure this value (1) is not null (2) is base64 encoded, and (3) represents a json document with the required keys

Warning

We will need to ensure that this SERVICE_ACCOUNT_CREDENTIALS environment variable exists where we deploy to.
I have no visibility of PROD or DEV environments, so cannot confirm this.

This environment variable needs to contain the Base64 encoded JSON document from google for the service account

This fixes a couple of issues:

1. We had non-idiomatic field names in `GoogleServiceConfiguration`
2. We were building JSON via toString()

To fix this, we use standard Java naming in the config object (Micronaut handles the dash or underscore replacement from config)

And we use `@JsonProperty` and Micronaut's `JsonMapper` to perform the serialization.

Removed the test for getters/setters working and replaced with a test that verifies config can be loaded, and converted to a json string and parsed.
@timyates timyates requested a review from mkimberlin June 27, 2024 10:39
@timyates timyates self-assigned this Jun 27, 2024

@NotNull
public String directory_id;
@JsonProperty("directory_id")
Copy link
Member

Choose a reason for hiding this comment

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

Not sure your plan for this PR after our discussion but, if you don't end up removing these completely, it's worth noting that you converted the _s to -s in the json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the yaml configuration I converted to use hyphens instead of underscores, but both will work for defining properties in Micronaut (but hyphen is more idiomatic)

In the generated JSON from this object, it will still use underscores (as the JsonProperty specifies underscores)

Copy link
Member

@mkimberlin mkimberlin left a comment

Choose a reason for hiding this comment

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

I'm going to go ahead and approve this. It's a positive step regardless of whether you end up eventually taking the step of removing these in favor of the encoded version.

@timyates timyates marked this pull request as draft July 1, 2024 13:29
@timyates
Copy link
Contributor Author

timyates commented Jul 1, 2024

I'll convert this to Draft until I get round to the Base64 decoding and simplification we discussed... It's next on my list once I get Graal working

@timyates timyates marked this pull request as ready for review July 2, 2024 15:20
@timyates timyates requested a review from mkimberlin July 2, 2024 15:28
Copy link
Collaborator

@rodecapd rodecapd left a comment

Choose a reason for hiding this comment

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

Discussed with Tim, he's goiing to change the ENV variable name and I'll pick it up in the terraform work.

@timyates timyates merged commit c476434 into develop Jul 3, 2024
@timyates timyates deleted the bugfix-sonar-use-regular-java-field-names branch July 3, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants