PYIC-7872: Add AppConfigService #3117
Conversation
MikeCollingwood
changed the title
|
Waiting on a devplatform PR to be implemented (and identity-platform PR to update to using the new |
deploy/template.yaml
Outdated
| Environment: | ||
| Variables: | ||
| ENVIRONMENT: !Sub "${Environment}" | ||
| CONFIG_SOURCE: "app-config" |
There was a problem hiding this comment.
Will change this to ssm before merging
| needs: check-if-api-tests-changed | ||
| if: ${{ needs.check-if-api-tests-changed.outputs.api-tests-changed == 'true' }} | ||
| uses: govuk-one-login/ipv-core-back/.github/workflows/secure-pipeline-api-tests-image.yml@main | ||
| uses: govuk-one-login/ipv-core-back/.github/workflows/secure-pipeline-api-tests-image.yml@pyic-7872 |
There was a problem hiding this comment.
Will change this main before merging. The changes to the workflow allow us to deploy API tests for the dev case
| import static com.fasterxml.jackson.core.JsonParser.Feature.STRICT_DUPLICATE_DETECTION; | ||
|
|
||
| public class YamlConfigService extends ConfigService { | ||
| public class YamlConfigService extends YamlParametersConfigService { |
There was a problem hiding this comment.
A lot of functionality was moved into this base class, shared with the AppConfigService class
| # Ensure the test report dir exists | ||
| [ -e "$TEST_REPORT_ABSOLUTE_DIR" ] && mkdir -p "$TEST_REPORT_ABSOLUTE_DIR" | ||
|
|
||
| ENVIRONMENT_SECRET=$(aws secretsmanager get-secret-value --secret-id ApiTestEnvironment | jq -r .SecretString) |
There was a problem hiding this comment.
Does this section need to be above line 13?
There was a problem hiding this comment.
The generate_traffic method is only called after this logic, so would be fine referencing TEST_ENV
| } | ||
| } | ||
|
|
||
| private void addJsonConfig(Map<String, String> map, JsonNode tree, String prefix) { |
There was a problem hiding this comment.
Using Json in this method name seems odd if we're dealing with YAML
There was a problem hiding this comment.
Ah yep I see the confusion, though the Yaml is parsed into JsonNode before. At this point we are dealing with Json-format config
...n-services/src/main/java/uk/gov/di/ipv/core/library/service/YamlParametersConfigService.java
Outdated
Show resolved
Hide resolved
| return super.getParametersByPrefix(path); | ||
| } | ||
|
|
||
| private void reloadParameters() { |
There was a problem hiding this comment.
Maybe call this updateParameters too for consistency?
There was a problem hiding this comment.
This method differs from updateParameters because of the fetch. I don't really mind but I think I got a PR comment last time the other way
libs/common-services/src/test/java/uk/gov/di/ipv/core/library/service/AppConfigServiceTest.java
Outdated
Show resolved
Hide resolved
libs/common-services/src/test/java/uk/gov/di/ipv/core/library/service/AppConfigServiceTest.java
Show resolved
Hide resolved
|
Glanced over but not planning to review in detail unless you'd like me to - it looks very similar to the original PR 😄 |
|
Proposed changes
What changed
Why did it change
Note
Issue tracking