feat: make docker compose startup time configurable #1166

IllTamer · 2023-12-01T20:21:25Z

Related Issue

Info about Issue or bug

Closes: #1065

Describe the changes you've made

Make docker compose startup time configurable for record, test and serve

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.

Screenshots (if any)

Original	Updated
original screenshot

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

github-actions · 2023-12-01T20:21:42Z

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

IllTamer · 2023-12-01T20:22:14Z

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

sweep-ai · 2023-12-01T20:22:29Z

Apply Sweep Rules to your PR?

Apply: All new business logic should have corresponding unit tests.
Apply: Refactor large functions to be more modular.
Apply: Add docstrings to all functions and file headers.

EraKin575 · 2023-12-02T15:26:17Z

cmd/record.go

@@ -206,6 +221,8 @@ func (r *Record) GetCmd() *cobra.Command {

 	recordCmd.Flags().Uint64P("delay", "d", 5, "User provided time to run its application")

+	recordCmd.Flags().Uint64P("buildDelay", "", 30, "User provided time to wait docker container build")


time would be a better datatype for inputting delay. Some docker builds can take minutes to startup. You can input it like that using Duration. Please make sure to change it everywhere. You can refer this: https://pkg.go.dev/time

got it, thanks. I will change it latter 😃

time would be a better datatype for inputting delay. Some docker builds can take minutes to startup. You can input it like that using Duration. Please make sure to change it everywhere. You can refer this: https://pkg.go.dev/time

Hey @EraKin575 , I want to set the type of buildDelay to string, so that we can use time.ParseDuration to parse the build time input by the user, such as "30s", "5m". Do you think this is a better idea than changing the type of buildDelay from uint64 to duration ?

The string type is only used to process user input. After initialization, the buildDelay variable will use the time.Duration type to participate in parameter transfer, time judgment and recording configuration.

I made a mistake, in fact, Flags#GetDuration automatically parses the time expression entered by the user like 30s, I mistakenly thought that it only accepts numeric type input. The modification has been completed, thank you for your help !

You won't be able to configure time like '30m5s'. Even if you do, that would be unnecessary complexity into this

You won't be able to configure time like '30m5s'. Even if you do, that would be unnecessary complexity into this

That's right, I'll go ahead and set the buildDelay type to time.Duration and specify the parameter's default unit to be seconds. Then I'll modify the relevant directive help

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

EraKin575 · 2023-12-03T09:26:29Z

Please update the screenshot too

gouravkrosx · 2023-12-04T08:04:51Z

cmd/serve.go

@@ -66,6 +67,13 @@ func (s *Serve) GetCmd() *cobra.Command {
 				return
 			}

+			buildDelay, err := cmd.Flags().GetDuration("buildDelay")


Right now, buildDelay is only limited to record and test command, please make the required changes.

@IllTamer Can we also use minute time unit in this flag? If yes please add an example of that also.

Sure, I will complete and modify later

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

gouravkrosx · 2023-12-05T06:26:02Z

cmd/test.go

@@ -229,6 +239,15 @@ func (t *Test) GetCmd() *cobra.Command {
 				}
 			}

+			if buildDelay <= time.Duration(30)*time.Second {


The same if check should be in the record command as well. And don't mention the example usage of record in the test command. It should be in the record command.

gouravkrosx · 2023-12-05T06:26:24Z

cmd/test.go

+				fmt.Printf("Warning: buildDelay is set to %d, incase your docker container takes more time to build use --buildDelay to set custom delay\n", buildDelay)
+				if isDockerCmd {
+					fmt.Println("Example usage:\n", `keploy test -c "docker run -p 8080:808 --network myNetworkName myApplicationImageName" --buildDelay 35s\n`, "\nor\n", `keploy test -c "docker run -p 8080:808 --network myNetworkName myApplicationImageName" --buildDelay 1m\n`)
+				} else {


Only mention test command usage here. And the port forwarding in the docker command should have port -p 8080:8080, please fix this if any other place also has this incorrect port.

Sorry for the confusion, what does 'Only mention test command usage here. ' mean , Shall I remove the warning message 🤔

Sorry, I didn't notice. You don't need to remove the command or any warning message. Just fix the port forwarding.

gouravkrosx · 2023-12-05T06:38:27Z

pkg/service/serve/serve.go

@@ -43,7 +43,7 @@ func NewServer(logger *zap.Logger) Server {
 const defaultPort = 6789

 // Serve is called by the serve command and is used to run a graphql server, to run tests separately via apis.
-func (s *server) Serve(path string, proxyPort uint32, testReportPath string, Delay uint64, pid, port uint32, lang string, passThroughPorts []uint, apiTimeout uint64, appCmd string) {
+func (s *server) Serve(path string, proxyPort uint32, testReportPath string, Delay uint64, BuildDelay time.Duration, pid, port uint32, lang string, passThroughPorts []uint, apiTimeout uint64, appCmd string) {


Please remove buildDelay flag from serve command, Because right not serve command is not supported for docker env.

gouravkrosx · 2023-12-05T06:40:54Z

pkg/service/serve/graph/schema.resolvers.go

@@ -61,7 +62,7 @@ func (r *mutationResolver) RunTestSet(ctx context.Context, testSet string) (*mod
 	go func() {
 		defer utils.HandlePanic()
 		r.Logger.Debug("starting testrun...", zap.Any("testSet", testSet))
-		tester.RunTestSet(testSet, testCasePath, testReportPath, "", "", "", delay, pid, ys, loadedHooks, testReportFS, testRunChan, r.ApiTimeout, ctx, nil, serveTest)
+		tester.RunTestSet(testSet, testCasePath, testReportPath, "", "", "", delay, buildDelay, pid, ys, loadedHooks, testReportFS, testRunChan, r.ApiTimeout, ctx, nil, serveTest)


Just pass some default value for buildDelay. Anyway, it will not be used in case of serve command. Because if it is a serve command then we don't even start the application by ourselves.

gouravkrosx · 2023-12-05T06:46:13Z

@IllTamer Please address the updated comments & also please update the template of the config file as well.

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

gouravkrosx · 2023-12-06T16:58:54Z

cmd/serve.go

@@ -66,6 +65,11 @@ func (s *Serve) GetCmd() *cobra.Command {
 				return
 			}

+			if err != nil {


You can remove this code since there is no build-delay in the serve command. So no need to check for this error.

gouravkrosx · 2023-12-06T17:08:16Z

cmd/test.go

+			}
+
+			if buildDelay <= 30*time.Second {
+				fmt.Printf("Warning: buildDelay is set to %d, incase your docker container takes more time to build use --buildDelay to set custom delay\n", buildDelay)


This warning should be printed only when there is docker command because we don't need the build-delay flag in case of native integration.

gouravkrosx · 2023-12-06T17:57:56Z

@IllTamer Please address the updated comments & resolve the merger conflicts, the rest looks good. And can I use a value like 1m40s in the build-delay flag

AkashKumar7902 · 2023-12-07T13:03:00Z

Hey @IllTamer
Please make a commit to this PR 280

Sonichigo · 2023-12-08T08:30:46Z

@IllTamer can you pease resolve the issues and update the PR!

IllTamer · 2023-12-08T08:46:37Z

@IllTamer can you pease resolve the issues and update the PR!

There are some things that are delaying me. I will update as soon as possible. : )

IllTamer · 2023-12-08T08:56:42Z

Please address the updated comments & resolve the merger conflicts, the rest looks good. And can I use a value like 1m40s in the build-delay flag

Yes, you can. It depends on the Flags#GetDuration function of the cobra library, and this method relies on the implementation of the pflag library, so you can enter parameters such as 1m20s or 1m61s

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

IllTamer · 2023-12-08T09:34:11Z

Hey @IllTamer Please make a commit to this PR 280

Sorry, but I'm not sure how to submit in your PR, it says I don't have permission. You can directly add a buildDelay: 30s after the record and test command configuration items delay to complete the update.

gouravkrosx · 2023-12-10T15:34:56Z

cmd/test.go

+
+			if isDockerCmd && buildDelay <= 30*time.Second {
+				fmt.Printf("Warning: buildDelay is set to %d, incase your docker container takes more time to build use --buildDelay to set custom delay\n", buildDelay)
+				if isDockerCmd {


No need to check for isDockerCmd again.

gouravkrosx · 2023-12-10T15:36:04Z

cmd/test.go

+			if isDockerCmd && buildDelay <= 30*time.Second {
+				fmt.Printf("Warning: buildDelay is set to %d, incase your docker container takes more time to build use --buildDelay to set custom delay\n", buildDelay)
+				if isDockerCmd {
+					fmt.Println("Example usage:\n", `keploy test -c "docker-compose up --build" --buildDelay 35s\n`, "\nor\n", `keploy test -c "docker-compose up --build" --buildDelay 1m\n`)
 				} else {
 					fmt.Println("Example usage:\n", cmd.Example)


Since this buildDelay flag is only in the case of the docker command, there is no need to add this example here.

Since this buildDelay flag is only in the case of the docker command, there is no need to add this example here.

By default, do we assume that the docker environment is built when the user uses the record command? Do we consider the situation where the user needs to rebuild when running the test command? Then the buildDelay parameter still needs to be checked

@IllTamer Whenever there is a docker env the isDockerCmd is true since the keploy Dockerfile contains this as an environment variable. So if you've checked that it is dockerCmd & the buildDelay is <=30 then just show the warning as the usage rather than showing another example.

gouravkrosx · 2023-12-10T15:37:10Z

@IllTamer Please resolve the merge conflicts and address the updated comments.

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

gouravkrosx · 2023-12-10T17:03:09Z

@IllTamer Please resolve the merger conflicts.

gouravkrosx · 2023-12-10T17:10:37Z

cmd/record.go

@@ -172,6 +182,15 @@ func (r *Record) GetCmd() *cobra.Command {
 				// user provided the absolute path
 			}

+			if buildDelay <= 30*time.Second {


Please remove the other example here also. And have you tested your changes with the config file as well?

gouravkrosx · 2023-12-10T17:13:29Z

@IllTamer The build is failing, please fix that.

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

gouravkrosx

🚀

IllTamer added 2 commits December 2, 2023 04:14

feat: make docker compose startup time configurable

37a7fbd

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

Merge branch 'keploy:main' into main

51d4a21

github-actions bot reviewed Dec 1, 2023

View reviewed changes

slayerjain added a commit that referenced this pull request Dec 1, 2023

@IllTamer has signed the CLA from Pull Request #1166

28e891c

IllTamer mentioned this pull request Dec 2, 2023

[bug]: failed to find container error #1167

Closed

1 task

EraKin575 reviewed Dec 2, 2023

View reviewed changes

refactor: change the type of buildDelay

85f5784

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

charankamarapu requested a review from gouravkrosx December 4, 2023 05:26

gouravkrosx reviewed Dec 4, 2023

View reviewed changes

IllTamer added 2 commits December 4, 2023 23:50

feat: removed buildDelay Flag in serve directive

c053020

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

feat: modified and improved the usage example of buildDelay Flag

ac14827

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

IllTamer requested a review from gouravkrosx December 4, 2023 15:53

gouravkrosx reviewed Dec 5, 2023

View reviewed changes

IllTamer added 3 commits December 6, 2023 01:32

feat: update the template of config file

b456e50

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

fix: remove buildDelay parameter in serve command

547292a

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

fix: fixed error port in example

0e64d84

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

IllTamer requested a review from gouravkrosx December 5, 2023 17:38

Merge branch 'main' into main

af6f371

gouravkrosx reviewed Dec 6, 2023

View reviewed changes

IllTamer added 2 commits December 8, 2023 17:05

fix: fixed remaining issues

28326aa

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

Merge branch 'main' of https://github.com/IllTamer/keploy

08278ba

IllTamer requested a review from gouravkrosx December 8, 2023 09:06

Merge branch 'main' into main

1c5db17

gouravkrosx reviewed Dec 10, 2023

View reviewed changes

IllTamer added 2 commits December 11, 2023 00:58

fix: remove unnecessary check

01e89cc

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

Merge branch 'main' of https://github.com/IllTamer/keploy

514a195

Merge branch 'main' into main

661ed98

IllTamer requested a review from gouravkrosx December 10, 2023 17:08

gouravkrosx reviewed Dec 10, 2023

View reviewed changes

fix: merge conflict error

feece2c

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

IllTamer requested a review from gouravkrosx December 10, 2023 17:27

fix: remove unnecessary check in record

8cf7e98

Signed-off-by: IllTamer <78360471+IllTamer@users.noreply.github.com>

gouravkrosx approved these changes Dec 10, 2023

View reviewed changes

gouravkrosx merged commit b623c2a into keploy:main Dec 10, 2023
10 checks passed

github-actions bot locked and limited conversation to collaborators Dec 10, 2023

		@@ -206,6 +221,8 @@ func (r Record) GetCmd() cobra.Command {

		recordCmd.Flags().Uint64P("delay", "d", 5, "User provided time to run its application")

		recordCmd.Flags().Uint64P("buildDelay", "", 30, "User provided time to wait docker container build")

feat: make docker compose startup time configurable #1166

feat: make docker compose startup time configurable #1166

Conversation

IllTamer commented Dec 1, 2023 • edited

Related Issue

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:

Screenshots (if any)

github-actions bot commented Dec 1, 2023 • edited

github-actions bot left a comment

Choose a reason for hiding this comment

IllTamer commented Dec 1, 2023

sweep-ai bot commented Dec 1, 2023

Apply Sweep Rules to your PR?

EraKin575 Dec 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IllTamer Dec 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EraKin575 commented Dec 3, 2023

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

gouravkrosx Dec 5, 2023 • edited

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

gouravkrosx Dec 5, 2023 • edited

Choose a reason for hiding this comment

gouravkrosx commented Dec 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gouravkrosx commented Dec 6, 2023 • edited

AkashKumar7902 commented Dec 7, 2023

Sonichigo commented Dec 8, 2023

IllTamer commented Dec 8, 2023

IllTamer commented Dec 8, 2023 • edited

IllTamer commented Dec 8, 2023

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

gouravkrosx commented Dec 10, 2023

gouravkrosx commented Dec 10, 2023

gouravkrosx Dec 10, 2023 • edited

Choose a reason for hiding this comment

gouravkrosx commented Dec 10, 2023

gouravkrosx left a comment

Choose a reason for hiding this comment

IllTamer commented Dec 1, 2023 •

edited

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

github-actions bot commented Dec 1, 2023 •

edited

EraKin575 Dec 2, 2023 •

edited

IllTamer Dec 2, 2023 •

edited

gouravkrosx Dec 5, 2023 •

edited

gouravkrosx Dec 5, 2023 •

edited

gouravkrosx commented Dec 5, 2023 •

edited

gouravkrosx commented Dec 6, 2023 •

edited

IllTamer commented Dec 8, 2023 •

edited

gouravkrosx Dec 10, 2023 •

edited