Skip to content

Conversation

tdgroot
Copy link
Member

@tdgroot tdgroot commented Nov 28, 2022

Depends on and needs to be merged after #65.

This allows brancher nodes to be cleaned up by labels. Hypernode Deploy will look up existing brancher nodes using the API, match them with the labels of the given stage and delete them.

This is an explicit feature, so it is necessary to call hypernode-deploy cleanup <target> on a specific point of the pipeline configuration (for example after a PR is closed).

@tdgroot tdgroot changed the title Create Brancher node with provided labels and settings Cleanupo Brancher nodes by labels Nov 28, 2022
@tdgroot tdgroot changed the title Cleanupo Brancher nodes by labels Cleanup Brancher nodes by labels Nov 28, 2022
Copy link
Contributor

@AlexanderGrooff AlexanderGrooff left a comment

Choose a reason for hiding this comment

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

I see you're changing some of the configuration loading. Is that necessary for specifying extra input for labels?

Comment on lines 57 to 80
/** @var string $stageName */
$stageName = $input->getArgument('stage');
if ($stageName) {
$config = $this->configurationLoader->load(
$input->getOption('file') ?: 'deploy.php'
);
foreach ($config->getStages() as $stage) {
if ($stage->getName() !== $stageName) {
continue;
}
foreach ($stage->getServers() as $server) {
if (!($server instanceof BrancherServer)) {
continue;
}
$labels = $server->getLabels();
$hypernode = $server->getOptions()[Server::OPTION_HN_PARENT_APP];
$brancherHypernodes = $this->brancherHypernodeManager->queryBrancherHypernodes(
$hypernode,
$labels
);
$this->brancherHypernodeManager->cancel(...$brancherHypernodes);
}
}
}
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 refactor it in a method cancelBasedOnLabel or something to keep this method concise

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion! Will do

@tdgroot
Copy link
Member Author

tdgroot commented Nov 28, 2022

I see you're changing some of the configuration loading. Is that necessary for specifying extra input for labels?

The configuration loading was moved into a separate class and I also removed the if fail then do composer install and retry, because it's really unclear what use case we're covering there. So it's way simpler now and also reusable in other components.

Base automatically changed from label_support to master December 5, 2022 14:29
@tdgroot tdgroot merged commit c9d7444 into master Dec 5, 2022
@tdgroot tdgroot deleted the cleanup_by_labels branch December 5, 2022 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants