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

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

Merged
merged 4 commits into from
Jul 3, 2024

Conversation

ashovlin
Copy link
Member

@ashovlin ashovlin commented Jun 20, 2024

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.

@ashovlin ashovlin requested review from normj, 96malhar and philasmar June 20, 2024 22:26
…w cache valid results, only check what is needed for the selected recipe, and add a timeout to prevent runaway commands.
@ashovlin ashovlin force-pushed the shovlia/capabilities-caching branch from ee15007 to 21125fd Compare June 20, 2024 22:34
@ashovlin ashovlin changed the title Rework System Capabilities Check (Caching, Timeout, and Based on Recipe) Rework System Capabilities Check (Caching, Timeout, and Conditional on Recipe) Jun 20, 2024
Comment on lines +48 to +49
bool needAwsCredentials = false,
CancellationToken cancellationToken = default)
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link

codecov bot commented Jun 20, 2024

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 ⚠️
src/AWS.Deploy.CLI/Commands/DeployCommand.cs 50.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@philasmar philasmar left a comment

Choose a reason for hiding this comment

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

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

@ashovlin
Copy link
Member Author

ashovlin commented Jun 28, 2024

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.

[Fact]
public async Task ContainerOnlyRecipe_DockerInWindowsMode_NoCache()
{
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
// Docker only supports Windows vs. Linux mode when running on Windows, so
// this test isn't valid on other OSes since we always assume "linux"
return;
}

@ashovlin ashovlin requested a review from philasmar June 28, 2024 19:28
Copy link
Contributor

@philasmar philasmar left a comment

Choose a reason for hiding this comment

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

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

@ashovlin
Copy link
Member Author

ashovlin commented Jul 2, 2024

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

Wrote a summary up in #837 (comment), 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 3723451

@ashovlin ashovlin requested review from 96malhar and philasmar July 2, 2024 15:13
@ashovlin ashovlin merged commit 5b1c4c2 into dev Jul 3, 2024
11 checks passed
@ashovlin ashovlin deleted the shovlia/capabilities-caching branch July 3, 2024 13:50
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.

3 participants