Fix broken links section in docker-compose recipe #2850
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -352,24 +352,24 @@ public Instance startMachine(String workspaceId, | |
| machine.setId(service.getId()); | ||
|
|
||
| machineStarter = (machineLogger, machineSource) -> { | ||
| CheServiceImpl serviceWithNormalizedSource = normalizeServiceSource(service, machineSource); | ||
|
|
||
| normalize(namespace, | ||
| workspaceId, | ||
| machineConfig.getName(), | ||
| serviceWithNormalizedSource); | ||
|
|
||
| ExtendedMachineImpl extendedMachine = new ExtendedMachineImpl(); | ||
| extendedMachine.setAgents(agents); | ||
| applyAgents(extendedMachine, serviceWithNormalizedSource); | ||
|
|
||
| return machineProvider.startService(namespace, | ||
| workspaceId, | ||
| environmentHolder.name, | ||
| machineConfig.getName(), | ||
| machineConfig.isDev(), | ||
| environmentHolder.networkId, | ||
| serviceWithNormalizedSource, | ||
| machineLogger); | ||
| }; | ||
| } else { | ||
|
|
@@ -667,6 +667,7 @@ private void startEnvironmentQueue(String namespace, | |
| try { | ||
| machineProvider.createNetwork(networkId); | ||
|
|
||
| final List<CheServiceImpl> launchedServices = new ArrayList<>(); | ||
| String machineName = queuePeekOrFail(workspaceId); | ||
| while (machineName != null) { | ||
| boolean isDev = devMachineName.equals(machineName); | ||
|
|
@@ -691,18 +692,19 @@ private void startEnvironmentQueue(String namespace, | |
| format("Environment of workspace with ID '%s' failed due to internal error", workspaceId)); | ||
| } | ||
|
|
||
| service.setName(machineName); | ||
| // needed to reuse startInstance method and | ||
| // create machine instances by different implementation-specific providers | ||
| MachineStarter machineStarter = (machineLogger, machineSource) -> { | ||
| CheServiceImpl serviceWithNormalizedSource = normalizeServiceSource(service, machineSource); | ||
| replaceServiceToContainerNameInLinks(serviceWithNormalizedSource, launchedServices); | ||
| return machineProvider.startService(namespace, | ||
| workspaceId, | ||
| envName, | ||
| serviceWithNormalizedSource.getName(), | ||
| isDev, | ||
| networkId, | ||
| serviceWithNormalizedSource, | ||
| machineLogger); | ||
| }; | ||
|
|
||
|
|
@@ -775,6 +777,7 @@ private void startEnvironmentQueue(String namespace, | |
| "' start interrupted. Workspace stopped before all its machines started"); | ||
| } | ||
|
|
||
| launchedServices.add(service); | ||
| machineName = queuePeekOrFail(workspaceId); | ||
| } | ||
| } catch (RuntimeException | ServerException e) { | ||
|
|
@@ -887,32 +890,32 @@ Instance startMachine(LineConsumer machineLogger, | |
| NotFoundException; | ||
| } | ||
|
|
||
| private CheServiceImpl normalizeServiceSource(CheServiceImpl service, | ||
| MachineSource machineSource) | ||
| throws ServerException { | ||
| CheServiceImpl serviceWithNormalizedSource = service; | ||
| if (machineSource != null) { | ||
| serviceWithNormalizedSource = new CheServiceImpl(service); | ||
| if ("image".equals(machineSource.getType())) { | ||
| serviceWithNormalizedSource.setBuild(null); | ||
| serviceWithNormalizedSource.setImage(machineSource.getLocation()); | ||
| } else { | ||
| // dockerfile | ||
| serviceWithNormalizedSource.setImage(null); | ||
| if (machineSource.getContent() != null) { | ||
| serviceWithNormalizedSource.setBuild(new CheServiceBuildContextImpl(null, | ||
| null, | ||
| machineSource.getContent(), | ||
| null)); | ||
| } else { | ||
| serviceWithNormalizedSource.setBuild(new CheServiceBuildContextImpl(machineSource.getLocation(), | ||
| null, | ||
| null, | ||
| null)); | ||
| } | ||
| } | ||
| } | ||
| return serviceWithNormalizedSource; | ||
| } | ||
|
|
||
| private Machine getMachineWithCorrectSource(MachineImpl machine, MachineSource machineSource) { | ||
|
|
@@ -930,6 +933,45 @@ private Machine getMachineWithCorrectSource(MachineImpl machine, MachineSource m | |
| return machineWithCorrectSource; | ||
| } | ||
|
|
||
| /** | ||
| * Replaces linked to this service's name with container name which represents the service in links section. | ||
| * The problem is that a user writes names of other services in links section in compose file. | ||
| * But actually links are constraints and they values should be names of containers (not services) to be linked. | ||
|
There was a problem hiding this comment. and they values => and their values |
||
| * <br/> | ||
| * For example: serviceDB:serviceDbAlias -> container_1234:serviceDbAlias <br/> | ||
| * If alias is omitted then service name will be used. | ||
| * | ||
| * @param serviceToStart | ||
| * service which is starting now | ||
| * @param launchedServices | ||
| * already running services | ||
| */ | ||
| @VisibleForTesting | ||
| void replaceServiceToContainerNameInLinks(CheServiceImpl serviceToStart, List<CheServiceImpl> launchedServices) { | ||
| final List<String> containerLinks = serviceToStart.getLinks(); | ||
|
There was a problem hiding this comment. we couldn't use Java8 stream/filter/collect for the whole method ? There was a problem hiding this comment. Yes, we can, but I think that it is redundant here. |
||
| for (int i = 0; i < containerLinks.size(); i++) { | ||
|
There was a problem hiding this comment. You could probably use for each here ( There was a problem hiding this comment. No, I cannot. I need to change the list. I can use |
||
| String link = containerLinks.get(i); | ||
|
There was a problem hiding this comment. Extracting the body of the for loop in a new method would make it more readable There was a problem hiding this comment. Taking into account, that we have only one loop in the method and this loop actually is method's body and body of this loop cannot be reused somewhere and if so, we should pass iterator and links list into new method as parameters, IMO, there is no point to extract loop body into another method. @garagatyi WDYT? There was a problem hiding this comment. Since you are going to refactor this code lets figure out what can be cleaned up after that refactoring |
||
| String serviceName; | ||
| String serviceAlias; | ||
| int colonIndex = link.indexOf(':'); | ||
| if (colonIndex != -1) { | ||
| serviceName = link.substring(0, colonIndex); | ||
| serviceAlias = link.substring(colonIndex + 1); | ||
|
There was a problem hiding this comment. What happen if user provide a (bad) link with an empty alias like: There was a problem hiding this comment.
There was a problem hiding this comment. If user provide link with empty alias, like |
||
| } else { | ||
| serviceName = link; | ||
| serviceAlias = null; | ||
| } | ||
| for (CheServiceImpl launchedService : launchedServices) { | ||
| if (serviceName.equals(launchedService.getName())) { | ||
| containerLinks.set(i, (serviceAlias == null) ? | ||
| launchedService.getContainerName() + ':' + serviceName : | ||
| launchedService.getContainerName() + ':' + serviceAlias); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void addMachine(MachineImpl machine) throws ServerException { | ||
| Instance instance = new NoOpMachineInstance(machine); | ||
| try (StripedLocks.WriteLock lock = stripedLocks.acquireWriteLock(machine.getWorkspaceId())) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep ipAddress as it was as method name is getIpAddress() not getIPAddress()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
ipAddressis conventional name , butiPAddressis needed for parsing value from json. Exactly likeiPPrefixLen,iPv6Gatewayin the same class.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @benoitf that iPAddress is weird. Wouldn't it possible to use
@SerializedNameannotation instead?