Add determineNetworkAlias function to simplify aliases creation #1617
Conversation
|
DAppNode bot has built and pinned the release to an IPFS node, for commit: f7cb715 This is a development version and should only be installed for testing purposes, install link (by dappnodebot/build-action) |
| @@ -35,7 +35,14 @@ export async function bitcoin( | |||
| ): Promise<ChainDataResult> { | |||
| const container = dnp.containers[0]; | |||
| if (!container) throw Error("no container"); | |||
| const containerDomain = getPrivateNetworkAlias(container); | |||
|
|
|||
| const {dnpName,serviceName} = container; | |||
There was a problem hiding this comment.
In order to avoid having to duplicate this code in every driver, I would add a function called build/generateNetworkAliasByPkg({service: string, pkg: InstalledPackageData})
It would check if the package is monoservice or the specified service is main to set the value of isMainOrMonoservice
Then, it would call build/generateNetworkAlias to get an available alias
| dnpName: dnpName, | ||
| serviceName: dnpName | ||
|
|
||
| const containerDomain = determineNetworkAlias({ |
There was a problem hiding this comment.
This does not look 100% reliable. What if you call this function with a dnp name that is a multiservice with no main service?
| @@ -38,19 +40,21 @@ export async function getEthConsClientApiUrl(dnpName: string): Promise<string> { | |||
| dnp.chain.serviceName | |||
| ) { | |||
| port = dnp.chain.portNumber; | |||
| domain = getPrivateNetworkAlias({ | |||
| domain = determineNetworkAlias({ | |||
There was a problem hiding this comment.
You could also call here build/generateNetworkAliasByPkg({service: string, pkg: InstalledPackageData})
There was a problem hiding this comment.
dnp.chain.serviceName does not seem 100% reliable either
| migrateCoreNetworkAndAliasInCompose(container, aliases); | ||
|
|
||
| // Adds aliases to the container network | ||
| for (const alias of aliases) { |
There was a problem hiding this comment.
You could try to avoid the loop here.
- Get the current alias in config
- Compare those to the new ones and leave only those to add
- Add them at the same time of possible
| const containerName = container.containerName; | ||
|
|
||
| // Wifi and VPN containers need a refresh connect due to their own network configuration | ||
| if (containerName === params.vpnContainerName || containerName === params.wifiContainerName) { |
There was a problem hiding this comment.
Are we sure only those? Are both wireguard and OpenVPN considered here?
| return Boolean( | ||
| endpointConfig && | ||
| endpointConfig.Aliases && | ||
| Array.isArray(endpointConfig.Aliases) && |
There was a problem hiding this comment.
Could it be just a string and not an array? Likely not, but just in case
| @@ -26,41 +26,48 @@ export async function getOptimismConfig(): Promise<OptimismConfigGet> { | |||
|
|
|||
| return { | |||
There was a problem hiding this comment.
This should not be here, you need to rebase carefully
| const containers = [containerMain, containerNotMain, monoContainer, containerMainPublic, containerNotMainPublic, monoContainerPublic]; | ||
|
|
||
| const CONTAINER_COMPOSE = ` | ||
| version: '3.4' |
There was a problem hiding this comment.
Here you set version 3.4, but above you migrate to 3.5 right? Why is this? I might be missing something
Marketen
force-pushed
the
pablo/the-endless-issue
branch
from
1d11b3c to
3bb4f81
Compare
|
Overridden by #1632 |
Add
determineNetworkAliasfunction to simplify aliases creation