New module parameters live-<xyz> #249
Conversation
|
This PR must be merged at the same as the one in the multiapps-controller |
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/DescriptorPlaceholderResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/DescriptorPlaceholderResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/DescriptorPlaceholderResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/DescriptorPlaceholderResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/DescriptorPlaceholderResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/DescriptorPlaceholderResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/DescriptorPlaceholderResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/DescriptorPlaceholderResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/DescriptorPlaceholderResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/DescriptorPlaceholderResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/DescriptorPlaceholderResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Show resolved
Hide resolved
| return idleToLiveParameterPairs.entrySet() | ||
| .stream() | ||
| .filter(idleToLiveParameterPair -> replaceableString.contains(idleToLiveParameterPair.getKey())) | ||
| .findFirst() |
There was a problem hiding this comment.
Not a style comment: What would happen if a parameter references 2 placeholders at once? e.g. '${default-host}-something-${default-uri}'; Not sure if this is even a real case but still
There was a problem hiding this comment.
they are resolved correctly
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/DescriptorPlaceholderResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/DescriptorPlaceholderResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Show resolved
Hide resolved
|
General question, is this new resolver really needed? Why are you resolving only the "config" section of the resource? |
|
...src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourceLiveParameterResolver.java
Show resolved
Hide resolved
...a/src/main/java/org/cloudfoundry/multiapps/mta/resolvers/v2/ResourcePlaceholderResolver.java
Show resolved
Hide resolved
| } | ||
|
|
||
| private String castToString(Object object) { | ||
| return String.valueOf(object); |
There was a problem hiding this comment.
is it possible to remove this method and use MiscUtils?
There was a problem hiding this comment.
if not, you make create a static util method in MiscUtils
|
|
||
| public class ResourceLiveParameterResolver implements Resolver<Resource> { | ||
|
|
||
| private static final String RESOURCE_PARAMETER_KEY_CONFIG = "config"; |
There was a problem hiding this comment.
I think it's ok to resolve all of the parameters, not only config
| public Resource resolve() { | ||
| if (resource.getParameters() | ||
| .containsKey(RESOURCE_PARAMETER_KEY_CONFIG)) { |
There was a problem hiding this comment.
Do you need to require the module from the resource in order for this resolution to work?
I think in the internal example, there is a missing requires section:
modules:
- name: foo
path: ./ui5Provider/buildresults.zip
type: custom
provides:
- name: foo
properties:
url: ~{default-url}
resources:
- name: bar
type: org.cloudfoundry.managed-service
requires:
- name: foo
parameters:
service: finch
service-plan: white
config:
foo-url: ~{foo/url}
There was a problem hiding this comment.
And if there is no requires to the provider module we should fail
There was a problem hiding this comment.
We will fail. If we don't fail here the next steps fails for sure
There was a problem hiding this comment.
We should have a check if the module is really required, and fail with a descriptive message
| .forEach(this::resolveResourceParameter); | ||
| } else if (entry.getValue() instanceof String) { | ||
| resolveConfigParameter(entry); |
There was a problem hiding this comment.
What will happen if this is an Array?
| public ResourceLiveParameterResolver(DeploymentDescriptor deploymentDescriptor, Resource resource, | ||
| Map<String, String> idleToLiveParameterPairs) { | ||
| this.deploymentDescriptor = deploymentDescriptor; |
There was a problem hiding this comment.
I also do not see if the resource properties are getting resolved?
A resource can have properties that are getting injected in env of the module that requires it
LMCROSSITXSADEPLOY-1187