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

Adding warning for uninstall --all while container runtime not running #1310

Merged
merged 3 commits into from
Jun 20, 2023

Conversation

mcaupybugs
Copy link
Contributor

Description

Added an else if condition to check the case when the dapr uninstall --all command is used but the container runtime is not running. In such a case, this code change will throw a warning stating that it could not remove the supporting containers.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #1288

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@mcaupybugs mcaupybugs requested review from a team as code owners June 9, 2023 17:21
@codecov
Copy link

codecov bot commented Jun 10, 2023

Codecov Report

Merging #1310 (93bd112) into master (fa2c99d) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1310      +/-   ##
==========================================
- Coverage   26.87%   26.82%   -0.06%     
==========================================
  Files          39       39              
  Lines        3873     3881       +8     
==========================================
  Hits         1041     1041              
- Misses       2758     2766       +8     
  Partials       74       74              
Impacted Files Coverage Δ
pkg/standalone/list.go 0.00% <0.00%> (ø)
pkg/standalone/uninstall.go 0.00% <0.00%> (ø)

Copy link
Member

@shubham1172 shubham1172 left a comment

Choose a reason for hiding this comment

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

Hi @mcaupybugs, this LGTM, could you also please add an e2e test? dapr init --slim followed by dapr uninstall --all should throw this error.

image

@mcaupybugs
Copy link
Contributor Author

Hey @shubham1172, I am not sure what E2E test should be written. Like I started to implement but after looking at the existing E2E tests I realized that each of those requires container runtime to be in the running state and is the first check done but my test case would require non-running container runtime. And if I try to manually give invalid container runtime using a --container-runtime flag then it will be blocked at the container-runtime validation step which is the IsValidContainerRuntime function and it checks if the runtime flag provided is either docker or podman. So not sure if i can even write E2E test for this scenario. please let me know wdyt

@shubham1172
Copy link
Member

shubham1172 commented Jun 17, 2023

Hi @mcaupybugs, you could stop the docker daemon using os/exec. Did you try that? Whilst doing it, we need to make sure that no other test is running in parallel. Although at this point I feel that it's non-trivial, so I am creating a separate issue (#1320) to track it and approving this PR. Thanks again for the PR! :)

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.

dapr uninstall --all should warn if container runtime is not running
3 participants