Fix service keys not renewed when existing service is used #1724
Conversation
| private List<DeployedMtaService> getManagedServices(List<DeployedMtaService> services) { | ||
| if (services == null) { | ||
| return null; | ||
| if (services != null) { |
There was a problem hiding this comment.
here it won't be null right?
There was a problem hiding this comment.
With the current implementation services can actually be null here. For example, when we deploy with no managed services, deployedMta.getServices() returns null and gets passed down. I will move the check earlier in the chain.
| List<DeployedMtaService> managed = getManagedServices(services); | ||
| if (!managed.isEmpty()) { | ||
| return managed.stream() | ||
| .map(s -> s.getGuid() != null ? s.getGuid() |
There was a problem hiding this comment.
when the guid is null?
| private List<String> extractManagedServiceGuids(List<DeployedMtaService> services) { | ||
| if (services != null) { | ||
| List<DeployedMtaService> managed = getManagedServices(services); | ||
| if (!managed.isEmpty()) { |
There was a problem hiding this comment.
inverse this if
| return List.of(); | ||
| } | ||
|
|
||
| return getServiceKeysByMetadataInternal(labelSelector, allServiceGuids); |
There was a problem hiding this comment.
why CustomControllerClientErrorHandler().handleErrorsOrReturnResult is removed?
There was a problem hiding this comment.
Somehow I missed it, I will add it back
| CloudServiceInstance instance = client.getServiceInstance(serviceName, false); | ||
| return instance != null ? instance.getGuid() | ||
| .toString() : null; |
There was a problem hiding this comment.
why should handle the optional scenario here. This will not fail if the service instance does not exist, it shouldn't fail only when it is optional or active: false.
| private List<String> getExistingServiceNamesFromDescriptor(ProcessContext context) { | ||
| DeploymentDescriptor descriptor = context.getVariable(Variables.DEPLOYMENT_DESCRIPTOR); | ||
|
|
||
| if (descriptor == null || descriptor.getResources() == null) { |
There was a problem hiding this comment.
when the descriptor is null? resources? empty or null?
There was a problem hiding this comment.
Descriptor is null when undeploy
|
|
||
| List<String> allServiceGuids = Stream.concat(managedGuids.stream(), existingServiceGuids.stream()) | ||
| .filter(Objects::nonNull) | ||
| .distinct() |
There was a problem hiding this comment.
do you need these distincts everywhere?
| } | ||
|
|
||
| private boolean isOptionalOrInactive(Resource resource) { | ||
| return Boolean.TRUE.equals(resource.isOptional()) || Boolean.FALSE.equals(resource.isActive()); |
There was a problem hiding this comment.
Why don't you just do resource.isOptional() || !resource.isActive()
| if (isOptionalOrInactive(resource)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Should we at least log that there was an exception or we must not print any information?
| return managedServices.isEmpty() ? null : managedServices; | ||
| return services.stream() | ||
| .filter(this::serviceIsNotUserProvided) | ||
| .collect(Collectors.toList()); |
| private boolean isOptionalOrInactive(Resource resource) { | ||
| return resource.isOptional() || !resource.isActive(); | ||
| } |
There was a problem hiding this comment.
I'd suggest to split this logic so it is clear if it is optional or inactive
...src/main/java/org/cloudfoundry/multiapps/controller/process/steps/DetectDeployedMtaStep.java
Show resolved
Hide resolved
...src/main/java/org/cloudfoundry/multiapps/controller/process/steps/DetectDeployedMtaStep.java
Outdated
Show resolved
Hide resolved
| .toString(); | ||
| } catch (CloudOperationException e) { | ||
| if (isOptionalOrInactive(resource)) { | ||
| getStepLogger().debug(Messages.IGNORING_NOT_FOUND_OPTIONAL_OR_INACTIVE_SERVICE, resource.getName()); |
There was a problem hiding this comment.
Add LOGGER.error here as well, because debug logs are usually not printed in Kibana
|
|
||
| private void logIgnoredService(String message, String serviceName) { | ||
| getStepLogger().debug(message, serviceName); | ||
| LOGGER.error(message, serviceName); |
There was a problem hiding this comment.
log the actual error as well, only in the error log
4 participants
No description provided.