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

#P39046999 Persist last (5) successful local deployments as part of CLI application code #164

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

leoSRM
Copy link

@leoSRM leoSRM commented Nov 14, 2022

Issue #, if available:

Description of changes:

Why is this change necessary:

How was this change tested:

Any additional information or context required to review the change:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@MikeDombo
Copy link
Member

Build is not successful, please address the errors.

MikeDombo
MikeDombo previously approved these changes Nov 23, 2022
@MikeDombo
Copy link
Member

Build is not successful. You must fix your commit messages

@github-actions
Copy link

github-actions bot commented Nov 29, 2022

Client Coverage Report

File Coverage Lines Branches
All files 73% 69% 76%
com.aws.greengrass.cli.module.AdapterModule 100% 100% 0%
com.aws.greengrass.cli.module.LogsModule 75% 75% 0%
com.aws.greengrass.cli.module.VersionProvider 13% 13% 0%
com.aws.greengrass.cli.util.FileUtils 0% 0% 0%
com.aws.greengrass.cli.util.FileUtils$1 0% 0% 0%
com.aws.greengrass.cli.util.PlatformUtils 0% 0% 0%
com.aws.greengrass.cli.adapter.impl.NucleusAdapterIpcClientImpl 9% 6% 11%
com.aws.greengrass.cli.adapter.impl.NucleusAdapterIpcClientImpl$1 0% 0% 0%
com.aws.greengrass.cli.CommandFactory 71% 71% 0%
com.aws.greengrass.cli.CLI 13% 13% 0%
com.aws.greengrass.cli.util.logs.LogFile 82% 90% 75%
com.aws.greengrass.cli.util.logs.FileReader 91% 85% 96%
com.aws.greengrass.cli.util.logs.LogEntry 91% 95% 87%
com.aws.greengrass.cli.util.logs.LogsUtil 92% 92% 0%
com.aws.greengrass.cli.util.logs.LogQueue 76% 85% 66%
com.aws.greengrass.cli.util.logs.impl.VisualizationImpl 89% 97% 81%
com.aws.greengrass.cli.util.logs.impl.FilterImpl 96% 100% 93%
com.aws.greengrass.cli.util.logs.impl.AggregationImpl 93% 98% 87%
com.aws.greengrass.cli.util.logs.impl.AggregationImplConfig 93% 93% 0%
com.aws.greengrass.cli.util.logs.impl.FilterImpl$FilterEntry 100% 100% 0%
com.aws.greengrass.cli.commands.PasswordCommand 16% 16% 0%
com.aws.greengrass.cli.commands.Logs 98% 96% 100%
com.aws.greengrass.cli.commands.BaseCommand 20% 20% 0%
com.aws.greengrass.cli.commands.ComponentCommand 100% 100% 100%
com.aws.greengrass.cli.commands.DeploymentCommand 72% 72% 71%

Minimum allowed coverage is 65%

Generated by 🐒 cobertura-action against f89da2a

@github-actions
Copy link

github-actions bot commented Nov 29, 2022

Server Coverage Report

File Coverage Lines Branches
All files 70% 90% 50%
com.aws.greengrass.cli.CLIEventStreamAgent 91% 96% 86%
com.aws.greengrass.cli.CLIEventStreamAgent$ListLocalDeploymentsHandler 73% 96% 50%
com.aws.greengrass.cli.CLIEventStreamAgent$ListComponentsHandler 97% 94% 100%
com.aws.greengrass.cli.CLIService$LocalDeploymentDetails 0% 0% 0%
com.aws.greengrass.cli.CLIEventStreamAgent$RestartComponentsHandler 98% 96% 100%
com.aws.greengrass.cli.CLIEventStreamAgent$CreateDebugPasswordHandler 91% 91% 0%
com.aws.greengrass.cli.CLIEventStreamAgent$LocalDeploymentDetails 81% 81% 0%
com.aws.greengrass.cli.CLIEventStreamAgent$GetComponentDetailsHandler 96% 96% 0%
com.aws.greengrass.cli.CLIEventStreamAgent$CreateLocalDeploymentHandler 82% 84% 80%
com.aws.greengrass.cli.CLIEventStreamAgent$GetLocalDeploymentStatusHandler 84% 94% 75%
com.aws.greengrass.cli.CLIService 79% 85% 73%
com.aws.greengrass.cli.CLIEventStreamAgent$StopComponentHandler 96% 96% 0%

Minimum allowed coverage is 58%

Generated by 🐒 cobertura-action against f89da2a

@@ -180,6 +185,27 @@ public void persistLocalDeployment(Topics serviceConfig, Map<String, Object> dep
Topics localDeploymentDetails = localDeployments.lookupTopics(deploymentId);
localDeploymentDetails.replaceAndWait(deploymentDetails);
// TODO: [P41178971]: Implement a limit on no of local deployments to persist status for
Copy link
Member

Choose a reason for hiding this comment

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

remove the TODO, this has been resolved now.

Comment on lines 561 to 563
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
Copy link
Member

Choose a reason for hiding this comment

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

do not catch in tests, just let the test method throw InterruptedException

}
cliEventStreamAgent.persistLocalDeployment(cliServiceConfig, localDeploymentDetails.convertToMapOfObject());
// to ensure topic add as order
Thread.sleep(1000);
Copy link
Member

Choose a reason for hiding this comment

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

this is going to slow things down, just sleep 1ms and use context.waitForPublishQueueToClear() after sleeping

@leoSRM
Copy link
Author

leoSRM commented Dec 5, 2022

Build is not successful. You must fix your commit messages

messages fixed

List<Topics> childrenToRemove = new ArrayList<>();
AtomicInteger deploymentSucceeded = new AtomicInteger();
localDeployments.forEach(topics -> {
Topics deploymentTopics = (Topics) topics;
Copy link
Member

Choose a reason for hiding this comment

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

check using instanceof before blindly casting to Topics.

Comment on lines 193 to 194
Topic existingChild = deploymentTopics.find(DEPLOYMENT_STATUS_KEY_NAME);
String value = existingChild.getOnce().toString();
Copy link
Member

Choose a reason for hiding this comment

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

use Coerce.toString(deploymentTopics.find(DEPLOYMENT_STATUS_KEY_NAME)

// if more than PERSIST_LIMIT deployments are success, remove until PERSIST_LIMIT left
if (deploymentSucceeded.get() > PERSIST_LIMIT) {
childrenToRemove.stream()
.sorted(Comparator.comparingLong(t -> t.find(DEPLOYMENT_STATUS_KEY_NAME).getModtime()))
Copy link
Member

Choose a reason for hiding this comment

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

find may return null, you must handle this correctly.

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.

2 participants