Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update docker alias migrations #1589

Closed
wants to merge 20 commits into from
Closed

Conversation

Marketen
Copy link
Contributor

No description provided.

@Marketen Marketen self-assigned this Sep 15, 2023
@github-actions github-actions bot temporarily deployed to commit September 15, 2023 19:30 Inactive
@github-actions
Copy link

github-actions bot commented Sep 15, 2023

@github-actions
Copy link

github-actions bot commented Sep 15, 2023

DAppNode bot has built and pinned the release to an IPFS node, for commit: a85751c

This is a development version and should only be installed for testing purposes, install link

/ipfs/QmeYLuMbiscbTco3mCewNxAK6NTcaAevXBbaHznz6VLg1P

(by dappnodebot/build-action)

@github-actions github-actions bot temporarily deployed to commit September 18, 2023 16:28 Inactive
@github-actions github-actions bot temporarily deployed to commit September 19, 2023 12:08 Inactive
@github-actions github-actions bot temporarily deployed to commit September 21, 2023 22:14 Inactive
@github-actions github-actions bot temporarily deployed to commit September 21, 2023 22:45 Inactive
@github-actions github-actions bot temporarily deployed to commit September 21, 2023 23:01 Inactive
@github-actions github-actions bot temporarily deployed to commit September 21, 2023 23:44 Inactive
@github-actions github-actions bot temporarily deployed to commit September 22, 2023 11:04 Inactive
@Marketen Marketen marked this pull request as ready for review September 22, 2023 11:14
@Marketen Marketen requested a review from a team as a code owner September 22, 2023 11:14
@Marketen Marketen force-pushed the marc/update-alias-migration branch from 638e576 to 768c78b Compare September 22, 2023 15:54
@github-actions github-actions bot temporarily deployed to commit September 22, 2023 16:00 Inactive
@github-actions github-actions bot temporarily deployed to commit September 22, 2023 16:19 Inactive
@github-actions github-actions bot temporarily deployed to commit September 22, 2023 16:37 Inactive
* and do docker inspect.
* and do docker inspect. This migration tries to assure that:
* Having a package name "example.dnp.dappnode.eth" the aliases should be:
* "example.dappnode" if the package is mono service
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we additionally supported service.example.dappnode for mono service packages?
@Marketen @3alpha

// Adds "root" alias to multiservice container if it is the main service
// "root" alias is always "dappnodename.dappnode", as if it was a mono service
if (isMainServiceOfMultiServicePackage(container)) {
aliasesToMigrate.push(getPrivateNetworkAlias({ dnpName: container.dnpName, serviceName: '' }));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure the function works as intended when passing serviceName=""

return false;
}

function updateEndpointConfig(currentEndpointConfig: Dockerode.NetworkInfo | null, alias: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would call this function addAliasToEndpointConfig

};
}

async function updateContainerNetwork(networkName: string, container: any, endpointConfig: Partial<Dockerode.NetworkInfo>): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe reconnectContainerToNetwork?

);

// 2. Get compose service network settings
const composeService = compose.services()[container.serviceName];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I liked this better, looks cleaner, but as you prefer

isCore: false,
};

const containers = [containerMain, containerNotMain, monoContainer];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are other cases right?

Public/Dnp
Monoservice/Multiservice
MainService/NoMainService

We might need to take all combinations into account (if there are equivalent combinations they can be omitted)

await shellSafe(`docker-compose -f ${TEST_ALIAS_PATH_MONO}/docker-compose.yml up -d`);

const [containerMainExists, containerNotMainExists, monoContainerExists, networkExists] = await Promise.all([
shellSafe(`docker container ls --filter name=${containerMain.containerName}`),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There must be a way to avoid repeating this line 3 times

}

await Promise.all([
shellSafe(`docker network connect ${DNCORE_NETWORK} ${containerMain.containerName}`),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

const monoContainerAliases = await getContainerAliasesOnNetwork(monoContainer.containerName, DNCORE_NETWORK);

// Define the expected aliases. These should match the aliases added by the `addAliasToGivenContainers` function.
const expectedMainAliases = ["mainService.logger.dappnode", "logger.dappnode"];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could skip this var declaration and set the values directly


after("Cleanup", async () => {
await Promise.all([
shellSafe(`docker stop ${containerMain.containerName}`),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, try to avoid repetition if possible

@github-actions github-actions bot temporarily deployed to commit September 25, 2023 09:11 Inactive
@github-actions github-actions bot temporarily deployed to commit September 25, 2023 09:28 Inactive
@github-actions github-actions bot temporarily deployed to commit September 25, 2023 09:39 Inactive
@github-actions github-actions bot temporarily deployed to commit September 25, 2023 12:51 Inactive
@github-actions github-actions bot temporarily deployed to commit September 25, 2023 12:55 Inactive
@github-actions github-actions bot temporarily deployed to commit September 25, 2023 14:48 Inactive
@Marketen Marketen force-pushed the marc/update-alias-migration branch from 0f4d52e to a2d9350 Compare September 25, 2023 14:55
@github-actions github-actions bot temporarily deployed to commit September 25, 2023 14:56 Inactive
@dappnodedev dappnodedev force-pushed the marc/update-alias-migration branch from a2d9350 to f87a247 Compare September 25, 2023 16:20
@github-actions github-actions bot temporarily deployed to commit September 25, 2023 16:26 Inactive
@dappnodedev dappnodedev linked an issue Sep 26, 2023 that may be closed by this pull request
@Marketen Marketen force-pushed the marc/update-alias-migration branch from f87a247 to a85751c Compare September 26, 2023 10:27
@github-actions github-actions bot temporarily deployed to commit September 26, 2023 10:32 Inactive
@github-actions github-actions bot temporarily deployed to commit September 26, 2023 10:33 Inactive
@dappnodedev
Copy link
Contributor

Overridden by #1632

@dappnodedev dappnodedev closed this Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add migration to ensure docker aliases consistency
3 participants