stop and remove docker container #874

tomargovind · 2023-09-25T12:47:27Z

Closes: #708

Describe the changes you've made

Earlier after stopping keploy, it used to stop the docker container but not remove it. Now, the container will be stopped and removed.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Code style update (formatting, local variables)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Please let us know if any test cases are added

Please describe the tests(if any). Provide instructions how its affecting the coverage.

Describe if there is any unusual behaviour of your code(Write `NA` if there isn't)

A clear and concise description of it.

Checklist:

My code follows the style guidelines of this project.
I have performed a self-review of my own code.
I have commented my code, particularly in hard-to-understand areas.
I have made corresponding changes to the documentation.
My changes generate no new warnings.
I have added tests that prove my fix is effective or that my feature works.
New and existing unit tests pass locally with my changes

github-actions · 2023-09-25T12:47:43Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

github-actions

Thank you and congratulations 🎉 for opening your very first pull request in keploy

tomargovind · 2023-09-25T12:51:25Z

I have read the CLA Document and I hereby sign the CLA

charankamarapu

Please resolve the mentioned issues

charankamarapu · 2023-09-25T15:48:44Z

pkg/hooks/loader.go

 // StopUserApplication stops the user application
 func (h *Hook) StopUserApplication() {
 	if h.userAppCmd != nil && h.userAppCmd.Process != nil {
 		h.logger.Debug("the process state for the user process", zap.String("state", h.userAppCmd.ProcessState.String()), zap.Any("processState", h.userAppCmd.ProcessState))
 		if h.userAppCmd.ProcessState != nil && h.userAppCmd.ProcessState.Exited() {
 			return
 		}
+
+		// Stop Docker Container and Remove it
+		h.StopAndRemoveDockerContainer()


we need to stop docker only if we know that application is ran via docker. In native case this function shouldn't be executed

And also if the user runs container and gives the container name or id then also we shouldn't stop the container.

charankamarapu · 2023-09-25T15:56:22Z

pkg/hooks/launch.go

@@ -25,6 +25,12 @@ import (
 	"go.keploy.io/server/pkg/models"
 )

+type DockerInfo struct {


This struct can be part of client package . Keeping it in this Package may cause circular dependency in future .

charankamarapu · 2023-09-25T15:58:33Z

pkg/hooks/loader.go

@@ -334,13 +342,48 @@ func (h *Hook) findAndCollectChildProcesses(parentPID string, pids *[]int) {
 	}
 }

+// Stop and Remove the docker container
+func (h *Hook) StopAndRemoveDockerContainer() {


This can be a function of dockerInfo struct or dockerClient instead of hooks. All docker related things can be moved to client package itself.

charankamarapu · 2023-09-25T16:04:25Z

pkg/hooks/launch.go

@@ -202,6 +210,7 @@ func (h *Hook) processDockerEnv(appCmd, appContainer, appNetwork string) error {
 			case e := <-messages:
 				if e.Type == events.ContainerEventType && e.Action == "create" {
 					// Fetch container details by inspecting using container ID to check if container is created
+					h.dockerDetails.containerID = e.ID


Use setter and getter functions instead of assigning it directly

charankamarapu · 2023-09-25T16:05:10Z

pkg/hooks/loader.go

+
+	container, err := dockerClient.ContainerInspect(context.Background(), containerID)
+    if err != nil {
+        fmt.Println(err)


Use logger instead of print statements

charankamarapu · 2023-09-25T16:05:58Z

pkg/hooks/loader.go

+	container, err := dockerClient.ContainerInspect(context.Background(), containerID)
+    if err != nil {
+        fmt.Println(err)
+        return


why is err not being returned..?

charankamarapu · 2023-09-25T16:06:16Z

pkg/hooks/loader.go

+
+    err = dockerClient.ContainerRemove(context.Background(), containerID, removeOptions)
+    if err != nil {
+        fmt.Println(err)


Use log statements

Signed-off-by: 404fixer <govindtomar94@gmail.com>

charankamarapu

Everything else looks good to me. Please correct the logger statement

charankamarapu · 2023-09-28T10:19:28Z

pkg/hooks/loader.go

+		// Stop Docker Container and Remove it if Keploy ran using docker
+		if len(h.idc.GetContainerID()) != 0  {
+			err := h.idc.StopAndRemoveDockerContainer()
+			if err != nil {


This should be an error log saying failed to stop container/ remove container , zap the error and suggest user to delete it manually

Signed-off-by: 404fixer <govindtomar94@gmail.com>

charankamarapu

Ship it!

re-Tick

LGTM

github-actions bot reviewed Sep 25, 2023

View reviewed changes

tomargovind requested review from re-Tick and charankamarapu September 25, 2023 12:48

slayerjain added a commit that referenced this pull request Sep 25, 2023

@404fixer has signed the CLA from Pull Request #874

e7d8503

charankamarapu reviewed Sep 25, 2023

View reviewed changes

tomargovind added 2 commits September 28, 2023 14:51

stop and remove docker container

6f3dee0

Signed-off-by: 404fixer <govindtomar94@gmail.com>

fixing code based on comments

ea5fd98

Signed-off-by: 404fixer <govindtomar94@gmail.com>

tomargovind requested a review from charankamarapu September 28, 2023 09:54

tomargovind force-pushed the stop-and-remove-docker branch from 59bfbee to ea5fd98 Compare September 28, 2023 10:15

charankamarapu reviewed Sep 28, 2023

View reviewed changes

fixing the error handling

c677d4e

Signed-off-by: 404fixer <govindtomar94@gmail.com>

tomargovind requested a review from charankamarapu September 28, 2023 11:16

fixed the error handling

fbeeeaa

Signed-off-by: 404fixer <govindtomar94@gmail.com>

charankamarapu approved these changes Sep 28, 2023

View reviewed changes

re-Tick approved these changes Sep 28, 2023

View reviewed changes

re-Tick merged commit 3948ecb into keploy:main Sep 28, 2023
7 checks passed

github-actions bot locked and limited conversation to collaborators Sep 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stop and remove docker container #874

stop and remove docker container #874

tomargovind commented Sep 25, 2023 •

edited

Loading

github-actions bot commented Sep 25, 2023 •

edited

Loading

github-actions bot left a comment

tomargovind commented Sep 25, 2023

charankamarapu left a comment

charankamarapu Sep 25, 2023

charankamarapu Sep 25, 2023

charankamarapu Sep 25, 2023

charankamarapu Sep 25, 2023

charankamarapu Sep 25, 2023

charankamarapu Sep 25, 2023

charankamarapu Sep 25, 2023

charankamarapu Sep 25, 2023

charankamarapu left a comment

charankamarapu Sep 28, 2023

charankamarapu left a comment

re-Tick left a comment

stop and remove docker container #874

stop and remove docker container #874

Conversation

tomargovind commented Sep 25, 2023 • edited Loading

Describe the changes you've made

Type of change

Please let us know if any test cases are added

Describe if there is any unusual behaviour of your code(Write NA if there isn't)

Checklist:

github-actions bot commented Sep 25, 2023 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

tomargovind commented Sep 25, 2023

charankamarapu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charankamarapu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charankamarapu left a comment

Choose a reason for hiding this comment

re-Tick left a comment

Choose a reason for hiding this comment

tomargovind commented Sep 25, 2023 •

edited

Loading

Describe if there is any unusual behaviour of your code(Write `NA` if there isn't)

github-actions bot commented Sep 25, 2023 •

edited

Loading