Adding ConfigProviderParameterManager implementation for GCP #3039
Conversation
psasidhar
force-pushed
the
gcp_config_provider
branch
2 times, most recently
from
fff22d6 to
232c24f
Compare
libs/java/server_gcp_common/pom.xml
Outdated
| <groupId>com.yahoo.athenz</groupId> | ||
| <artifactId>athenz-server-common</artifactId> | ||
| <version>${project.parent.version}</version> | ||
| <scope>compile</scope> |
There was a problem hiding this comment.
why is this compile only?
libs/java/server_gcp_common/pom.xml
Outdated
|
|
||
| <properties> | ||
| <code.coverage.min>1.00</code.coverage.min> | ||
| <code.coverage.min>0.97</code.coverage.min> |
There was a problem hiding this comment.
any reason we couldn't get 100%?
There was a problem hiding this comment.
I think this is because "Const" class and the ParameterManagerClientHelper class that has only static methods.
There was a problem hiding this comment.
updated it to get back to 100
| return null; | ||
| } | ||
| String location = System.getProperty(ATHENZ_PROP_GCP_LOCATION, GLOBAL_LOCATION); | ||
| String projectId = System.getProperty(ATHENZ_PROP_GCP_PROJECT_ID, ATHENZ_GCP_PROJECT_ID_DEFAULT); |
There was a problem hiding this comment.
why do we have a default project id? that just sounds wrong.
There was a problem hiding this comment.
I was trying to avoid GCP library throwing INVALID_ARGUMENT exception when the project id is empty, but handling this explicitly would make more sense.
Added code to throw IllegalArgumentException, and so this will surface up during server startup. only when ConfigProviderParameterManager is configured and the sourceDescription matches and yet the project_id is not set.
| } | ||
|
|
||
| public static ParameterVersion getLatestParameterVersion(ParameterManagerClient client, ParameterName parameterName) { | ||
| LOG.info("getLatestParameterVersion: {}", parameterName); |
There was a problem hiding this comment.
why do we have this info call - doesn't this going to be logged every minute for all the parameters all the time?
| } | ||
|
|
||
| public static ParameterVersion getLatestParameterVersion(ParameterManagerClient client, String projectId, String location, String parameter) { | ||
| LOG.info("getLatestParameterVersion: {}", parameter); |
There was a problem hiding this comment.
same comment as above. this seems like it will just flood the server log with unnecessary details.
psasidhar
force-pushed
the
gcp_config_provider
branch
from
232c24f to
57d098d
Compare
Signed-off-by: Sasi Palaka <[email protected]>
psasidhar
force-pushed
the
gcp_config_provider
branch
from
57d098d to
88c037e
Compare
2 participants
Description
Contribution Checklist:
Attach Screenshots (Optional)