search

aws / aws-dotnet-deploy

Opinionated tooling that simplifies deployment of .NET applications to AWS.
https://aws.github.io/aws-dotnet-deploy/
Apache License 2.0
138 stars 32 forks source link

Rework System Capabilities Check (Caching, Timeout, and Conditional on Recipe) #837

Closed ashovlin closed 2 months ago

ashovlin commented 2 months ago

Issue #, if available: DOTNET-7552

Description of changes: The deploy tool checks whether the user has Node.JS and Docker installed, since those are required for most deployments.

  1. Applied a timeout of one minute to these checks. We've seen cases where Docker doesn't wake up from resource saver mode, then the check hangs indefinitely.
  2. We now check selectively for each based on the selected, rather than always checking for both. For example, Elastic Beanstalk doesn't require Docker, and pushing an image to ECR doesn't require Node/CDK.
  3. We now cache successful results for an hour. We don't cache unsuccessful results, since a user may have taken an action (such as starting Docker or switching it to Linux mode) after a failed check.

I also consolidated two different helpers for unit testing CommandLineWrapper into one.

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

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 91.02564% with 7 lines in your changes missing coverage. Please review.

Project coverage is 61.85%. Comparing base (bca37f9) to head (3723451). Report is 1 commits behind head on dev.

Files Patch % Lines
....Deploy.Orchestration/SystemCapabilityEvaluator.cs 91.22% 2 Missing and 3 partials :warning:
src/AWS.Deploy.CLI/Commands/DeployCommand.cs 50.00% 0 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #837 +/- ## ========================================== + Coverage 61.57% 61.85% +0.27% ========================================== Files 278 279 +1 Lines 10801 10840 +39 Branches 1492 1496 +4 ========================================== + Hits 6651 6705 +54 + Misses 3612 3601 -11 + Partials 538 534 -4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ashovlin commented 2 months ago

Please add tests that go over the non-covered areas to address the CodeCov warnings

Fixed now. For the one outstanding warning, that's where we're reporting that Docker is in "windows" mode. I added a test for this, but it returns early if not running on Windows because we assume "linux" otherwise.

https://github.com/aws/aws-dotnet-deploy/blob/9db57582515d4fbcf54e356505654266900cd371/test/AWS.Deploy.Orchestration.UnitTests/SystemCapabilityEvaluatorTests.cs#L137-L145

ashovlin commented 2 months ago

I'm good with the changes, but Codecov still shows a warning. Could you please tackle that?

Wrote a summary up in https://github.com/aws/aws-dotnet-deploy/pull/837#issuecomment-2197500039, essentially those lines will only ever be invoked on Windows (it's when Docker is running in Windows mode, which isn't possible on Linux/OSX), so the test that I did add returns early. I don't think it's worth mocking out the OS detection just for this coverage.

Noticed another issue while doing final testing in Visual Studio, I didn't realize that SystemCapabilityEvaluator was being recreated per server mode API call, which defeats the point of the caching. Addressed in https://github.com/aws/aws-dotnet-deploy/pull/837/commits/3723451b970e68bda2ec9763356fca57938ceda0