Draft: AppConfig integration #473
Conversation
github-actions
bot
added
type: dependency-upgrade
MatejNedic
added
component: appconfig
| RequestContext.Builder builder = RequestContext.builder(); | ||
| AtomicReference<Integer> i = new AtomicReference<>(0); | ||
| builder.context(context); | ||
| Arrays.stream(context.split("___")).forEach(split -> { |
There was a problem hiding this comment.
Should the prefix that is splitting values be adjustable or changed to something else?
I think even a simple / could do but I am afraid there will be cases where someone has / in ConfigurationProfileName or ApplicationName
There was a problem hiding this comment.
I would go with the simple delimiter / and wait for feedback on the next milestone. Given you can have any character in the names. Also, for better debugging trace for loop would be enough
MatejNedic
requested review from
eddumelendez and
maciejwalkowiak
as code owners
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
My concern is related to the token management
From docs
This token should only be used once in your first call to GetLatestConfiguration. You must use the new token in the GetLatestConfiguration response (NextPollConfigurationToken) in each subsequent call to GetLatestConfiguration.
This token and the NextPollConfigurationToken token expire after 24 hours. If a GetLatestConfiguration call uses an expired token, the system returns BadRequestException.
Also, regarding to
Configuration
The data of the configuration. This may be empty if the client already has the latest version of configuration.
is the integration already managing this? I guess the token is the one that is used here and in every restart the app will generate a new token and will perform those request to fetch the latest configuration. However, what about refresh? I think the result is the same but just writing down some doubts I have regarding to this.
Source: https://docs.aws.amazon.com/appconfig/2019-10-09/APIReference/API_Operations_AWS_AppConfig_Data.html
| @@ -12,14 +12,17 @@ io.awspring.cloud.autoconfigure.dynamodb.DynamoDbAutoConfiguration | |||
| # ConfigData Location Resolvers | |||
| org.springframework.boot.context.config.ConfigDataLocationResolver=\ | |||
| io.awspring.cloud.autoconfigure.config.parameterstore.ParameterStoreConfigDataLocationResolver,\ | |||
| io.awspring.cloud.autoconfigure.config.appconfig.AppConfigDataLocationResolver,\ | |||
There was a problem hiding this comment.
order should be alphabetically. it applies for the other two entries
|
|
||
| public static RequestContext splitContext(String context) { | ||
| RequestContext.Builder builder = RequestContext.builder(); | ||
| AtomicReference<Integer> i = new AtomicReference<>(0); |
There was a problem hiding this comment.
any specific use? maybe I'm missing something
| RequestContext.Builder builder = RequestContext.builder(); | ||
| AtomicReference<Integer> i = new AtomicReference<>(0); | ||
| builder.context(context); | ||
| Arrays.stream(context.split("___")).forEach(split -> { |
There was a problem hiding this comment.
I would go with the simple delimiter / and wait for feedback on the next milestone. Given you can have any character in the names. Also, for better debugging trace for loop would be enough
.../src/test/java/io/awspring/cloud/autoconfigure/config/appconfig/AppConfigDataLoaderTest.java
Show resolved
Hide resolved
| * @author Matej Nedic | ||
| * @since 3.0 | ||
| */ | ||
| public class RequestContext { |
There was a problem hiding this comment.
From AppConfig perspective, there is known attributes such as configurationProfileIdentifier, environmentIdentifier and applicationIdentifier. However, context is something to be used internally AFAICS. I would suggest to use and internal format to be built once the other attributes have been filled.
There was a problem hiding this comment.
So we should pass String context which is full path and in AppConfigPropertySource split it up to configurationProfileIdentifier, environmentIdentifier and applicationIdentifier? If I understood it right. RequestContext does contain a full context String tho, but mb this approach follows a pattern that other integrations have.
MatejNedic
force-pushed
the
app-config-integration
branch
from
50d5a50 to
296ee18
Compare
MatejNedic
force-pushed
the
app-config-integration
branch
from
296ee18 to
8b65c57
Compare
# Conflicts: # spring-cloud-aws-autoconfigure/pom.xml
.../src/test/java/io/awspring/cloud/autoconfigure/config/appconfig/AppConfigDataLoaderTest.java
Outdated
Show resolved
Hide resolved
|
It would be cool to include tests with localstack once is implemented -> |
|
I'm also eagerly waiting for the feature. How can I help to get it integrated? |
|
any updates? |
|
This feature would be awesome! Keeping an eye on this, and willing to help if needed. |
|
I feel like I must have missed it but can you clarify what happens when a new AppConfig |
|
@MatejNedic Localstack implemented Appconfig support |
|
@maciejwalkowiak didn't know that perfect. Will implement with LocalStack tests and we can then hopefully merge and release. |
|
Hey @NChitty , if you enable reload inside of integration it will periodically call AppConfig and do a reload or refresh depending on what you choose. It is covered by documentation. |
|
I don't wanna be rude, but over a year has passed by since my last status request. Is this feature dead before arrival?! |
|
Hey @noshua , I am thinking about redoing this integration. I have to rethink it, don't like current implementation can do more things will be exploring it. |
📢 Type of change
📜 Description
Still have to go through it once more time and add some docs. Tried against real AWS and all tests are passing.
Since AWS Localstack has a bug it is not possible to make a real integration test.
Closes #465
@eddumelendez SInce you worked on AppConfig integration before please when you have time check the design. I will do more cleaning and polishing of the code.
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
🔮 Next steps