bug fix/1747. enable error logging for invalid flags #1753

ethanwater · 2024-03-28T22:00:43Z

Related Issue

fixes bug [feature]: log error in case of invalid flags #1747

Closes: #1747

Describe the changes you've made

Remove the silencing of Record.
Implement proper handling of ValidateFlags() calls.

Type of change

My code follows the code style of this project.
[x ] Bug fix (non-breaking change which fixes an issue)
[x ] New feature (non-breaking change which adds functionality)
[x ] 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

Signed-off-by: Ethan Water <angelshatepop@gmail.com>

github-actions · 2024-03-28T22:00:58Z

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

ethanwater · 2024-03-28T22:03:26Z

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

ethanwater · 2024-03-28T22:03:39Z

recheck

shivamsouravjha · 2024-03-29T07:16:27Z

do these fixes work, I tried the same them locally didn't work for me

if possible attach the screenshots

ethanwater · 2024-03-29T15:09:47Z

@shivamsouravjha , it works.

The code also works if the long variant of an incorrect flag is used, ex: --applekepapa.

I've reviewed the code, and it properly extinguishes the issue.

shivamsouravjha · 2024-03-29T16:45:28Z

cli/config.go

+			if err := cmdConfigurator.ValidateFlags(ctx, cmd); err != nil {
+				utils.LogError(logger, err, "failed to validate flags")
+				return err
+			}
+			return nil


what's the use of logerror?

This is proper use of handling errors according to Google's official Golang documentation. The purpose of it is to simply log errors following production standard.

Hopefully this helps you understand: https://go.dev/blog/error-handling-and-go

The same procedure is used at AWS and other established cloud-based companies using Go. I don't understand where your confusion lies.

I understand the use of said log function but they're not leading to expected results, I put a print function in utils.LogError function and it doesn't work. I wasn't able to see the said print statement as well as the logs.

Perhaps you can dig in more as to why this is happening

Eg:

The LogError is a function that is used throughout the entirety of the codebase and was designed by the developers.

**What you're looking at is outside the scope of the issue you posted. The but has been fixed and is ready to be merged. **

Also to trigger that, you would need to make ValidateFlags fail. The function is implemented in numerous instances.

If you have further questions about LogError() Ask the creators of Keploy themselves.

@shivamsouravjha it seems to be working fine for me. Please check if there is any env related issue.

@ethanwater thanks for the PR, It does work of me as you described. My initial thoughts :

There seems to some redundancy in the code. eg: utils.LogError(logger, err, "failed to validate flags") is added to both the record and test cmds. It would be better to add the log in one place (maybe somewhere inside the ValidateFlags method? ).

I personally feel that the error is not standing out enough. It seems to get lost with the usage instructions logged right after it. Would love to listen to some of your ideas on this. Some quick things I can think of is:

Add a new line after the error.

Maybe add an error emoji? like ❌or ⚠️ or something more fitting?

Add yellow or red color to the ERROR text? This can be good for console output but doesn't work where ANSI color is not supported. eg: piping to a file, downloading logs from a CI (GHA, Jenkins...)

@ethanwater @shivamsouravjha its totally cool to use the LogError wrapper. It's something we recently added to avoid logging unnecessary errors while ctx is cancelled. We might add more validations in the future.

Thank you @slayerjain for the review. I can implement these features. Should I implement these in the current PR or another one after merge?

I would really appreciate implementing in the current PR. Happy to actively share my thoughts if you need :)

@slayerjain Awesome, looking at it- I agree with these features. What do you think of this?

Signed-off-by: Ethan Water <watermonic@gmail.com>

ethanwater · 2024-03-29T23:16:14Z

@slayerjain I implemented the features you requested! I also removed the redundant logging as the previous logging is already substantial enough.

Note: I set the revamped error output in the AddFlags function due to it seeming to require the command.SetFlagErrorFunc() being called post initialization.

slayerjain · 2024-03-30T05:56:18Z

cli/provider/cmd.go

@@ -153,6 +154,15 @@ func NewCmdConfigurator(logger *zap.Logger, config *config.Config) *CmdConfigura
 }

 func (c *CmdConfigurator) AddFlags(cmd *cobra.Command) error {
+	//sets the displayment of flag-related errors
+	cmd.SilenceErrors = true
+	cmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error {


@ethanwater cmd variable here is unused, maybe replace with _

slayerjain · 2024-03-30T05:57:42Z

cli/config.go

@@ -24,10 +24,12 @@ func Config(ctx context.Context, logger *zap.Logger, cfg *config.Config, service
 		Short:   "manage keploy configuration file",
 		Example: "keploy config --generate --path /path/to/localdir",
 		PreRunE: func(cmd *cobra.Command, _ []string) error {
-			return cmdConfigurator.ValidateFlags(ctx, cmd)
+			if err := cmdConfigurator.ValidateFlags(ctx, cmd); err != nil {


since we are not doing anything here, it can be left unchanged to return cmdConfigurator.ValidateFlags(ctx, cmd) as in the previous commit.

slayerjain

This looks awesome Please just remove the redundant error check in the 3 commands as pointed by the linter too and the redundant cmd variable in cmd.SetFlagErrorFunc . We should be good to roll after that :)

slayerjain · 2024-03-30T06:07:49Z

cli/provider/cmd.go

+	//sets the displayment of flag-related errors
+	cmd.SilenceErrors = true
+	cmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error {
+		color.Red(fmt.Sprintf("❌ error: %v", err))


@ethanwater This is very nice! I really like the output now.

Signed-off-by: Ethan Water <watermonic@gmail.com>

ethanwater · 2024-03-30T06:14:54Z

@slayerjain Thanks again for the review! And I'm glad you like the new output! I have applied the changes you recommended.

slayerjain · 2024-03-30T06:16:17Z

@ethanwater Thanks for the contribution and patience. Looking forward to your next contribution :)

ethanwater added 2 commits March 28, 2024 17:54

display invalid-flags for record + error handling for ValidateFlags

ed1e22c

Signed-off-by: Ethan Water <angelshatepop@gmail.com>

cleanup

4cd2b86

Signed-off-by: Ethan Water <angelshatepop@gmail.com>

github-actions bot reviewed Mar 28, 2024

View reviewed changes

slayerjain added a commit that referenced this pull request Mar 28, 2024

@ethanwater has signed the CLA from Pull Request #1753

5045949

Merge branch 'keploy:main' into fix/1747

00a3e20

shivamsouravjha reviewed Mar 29, 2024

View reviewed changes

nehagup requested a review from slayerjain March 29, 2024 17:48

ethanwater added 2 commits March 29, 2024 19:03

aesthetic error output

1f088a5

Signed-off-by: Ethan Water <watermonic@gmail.com>

remove redundant logging

a6dfc17

Signed-off-by: Ethan Water <watermonic@gmail.com>

slayerjain reviewed Mar 30, 2024

View reviewed changes

slayerjain requested changes Mar 30, 2024

View reviewed changes

slayerjain reviewed Mar 30, 2024

View reviewed changes

ethanwater added 2 commits March 30, 2024 02:09

remove redundant code/ be nice to the linter!

c95462b

Signed-off-by: Ethan Water <watermonic@gmail.com>

replaced unused arg with _

3d86c64

Signed-off-by: Ethan Water <watermonic@gmail.com>

slayerjain approved these changes Mar 30, 2024

View reviewed changes

slayerjain merged commit 16eacd7 into keploy:main Mar 30, 2024
14 checks passed

github-actions bot locked and limited conversation to collaborators Mar 30, 2024

ethanwater deleted the fix/1747 branch March 30, 2024 06:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug fix/1747. enable error logging for invalid flags #1753

bug fix/1747. enable error logging for invalid flags #1753

ethanwater commented Mar 28, 2024

github-actions bot commented Mar 28, 2024 •

edited

Loading

github-actions bot left a comment

ethanwater commented Mar 28, 2024

ethanwater commented Mar 28, 2024

shivamsouravjha commented Mar 29, 2024

ethanwater commented Mar 29, 2024 •

edited

Loading

shivamsouravjha Mar 29, 2024

ethanwater Mar 29, 2024 •

edited

Loading

ethanwater Mar 29, 2024 •

edited

Loading

shivamsouravjha Mar 29, 2024 •

edited

Loading

ethanwater Mar 29, 2024 •

edited

Loading

slayerjain Mar 29, 2024 •

edited

Loading

slayerjain Mar 29, 2024

ethanwater Mar 29, 2024

slayerjain Mar 29, 2024

ethanwater Mar 29, 2024 •

edited

Loading

ethanwater commented Mar 29, 2024 •

edited

Loading

slayerjain Mar 30, 2024 •

edited

Loading

slayerjain Mar 30, 2024

slayerjain left a comment •

edited

Loading

slayerjain Mar 30, 2024

ethanwater commented Mar 30, 2024

slayerjain commented Mar 30, 2024

bug fix/1747. enable error logging for invalid flags #1753

bug fix/1747. enable error logging for invalid flags #1753

Conversation

ethanwater commented Mar 28, 2024

Related Issue

Describe the changes you've made

Type of change

github-actions bot commented Mar 28, 2024 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

ethanwater commented Mar 28, 2024

ethanwater commented Mar 28, 2024

shivamsouravjha commented Mar 29, 2024

ethanwater commented Mar 29, 2024 • edited Loading

shivamsouravjha Mar 29, 2024

Choose a reason for hiding this comment

ethanwater Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

ethanwater Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

shivamsouravjha Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

ethanwater Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

slayerjain Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

slayerjain Mar 29, 2024

Choose a reason for hiding this comment

ethanwater Mar 29, 2024

Choose a reason for hiding this comment

slayerjain Mar 29, 2024

Choose a reason for hiding this comment

ethanwater Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

ethanwater commented Mar 29, 2024 • edited Loading

slayerjain Mar 30, 2024 • edited Loading

Choose a reason for hiding this comment

slayerjain Mar 30, 2024

Choose a reason for hiding this comment

slayerjain left a comment • edited Loading

Choose a reason for hiding this comment

slayerjain Mar 30, 2024

Choose a reason for hiding this comment

ethanwater commented Mar 30, 2024

slayerjain commented Mar 30, 2024

github-actions bot commented Mar 28, 2024 •

edited

Loading

ethanwater commented Mar 29, 2024 •

edited

Loading

ethanwater Mar 29, 2024 •

edited

Loading

ethanwater Mar 29, 2024 •

edited

Loading

shivamsouravjha Mar 29, 2024 •

edited

Loading

ethanwater Mar 29, 2024 •

edited

Loading

slayerjain Mar 29, 2024 •

edited

Loading

ethanwater Mar 29, 2024 •

edited

Loading

ethanwater commented Mar 29, 2024 •

edited

Loading

slayerjain Mar 30, 2024 •

edited

Loading

slayerjain left a comment •

edited

Loading