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

e2e: Fix improper use of formatting #1622

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alonsofabila-dev
Copy link

@alonsofabila-dev alonsofabila-dev commented Oct 28, 2024

When using fmt.Errorf(), we don't need to format the message with
fmt.Sprintf().

Fixes #1618

Copy link
Member

@nirs nirs left a comment

Choose a reason for hiding this comment

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

Thansk @alonsofabila-dev!

Few issues:

  • Use %q instead of %s

  • No need to split to commits by file, you can squash the commits to single commit.

  • Add this header to the commit message:

    Fixes: #1618
    

e2e/dractions/retry.go Outdated Show resolved Hide resolved
Copy link
Member

@nirs nirs left a comment

Choose a reason for hiding this comment

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

One more thing - not clear what is the (chore) prefix. Check recent commits for the style used in this project.

@alonsofabila-dev alonsofabila-dev force-pushed the fix/Improper-use-of-error-formatting branch from 34cbef2 to 9d4bce6 Compare October 28, 2024 22:07
Copy link
Member

@nirs nirs left a comment

Choose a reason for hiding this comment

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

The actual change look good now, but we have 3 commits for for the same logical change. We want to have a single commit in git history for this issue.

Replacing %s with %q can be considered a different change, and maybe it is better done for the entire e2e package. This will be more work since lot of code use string concatenation ("a " + b + " c") instead of formatting which makes it harder to quote values.

To keep this PR simple and complete it quickly, lets remove the last commit and work on quoting later.

The first 2 commits should be squashed using git rebase. If you never did this see #1623.

The commit message have headers (like Signed-off-by: alonso fabila [email protected]). We want another header with (Fixes: #1618). You add the header as the commit message title in the last commit.

@alonsofabila-dev alonsofabila-dev force-pushed the fix/Improper-use-of-error-formatting branch 2 times, most recently from 8489534 to 2262579 Compare October 29, 2024 15:44
@alonsofabila-dev
Copy link
Author

donde

@nirs
Copy link
Member

nirs commented Oct 29, 2024

@alonsofabila-dev thanks for the update. If you don't mind I'll fix the commit message myself.

@nirs nirs force-pushed the fix/Improper-use-of-error-formatting branch from 2262579 to 59a9b36 Compare October 29, 2024 15:54
@nirs nirs changed the title Fix/improper use of error formatting e2e: Fix improper use of formatting Oct 29, 2024
@nirs
Copy link
Member

nirs commented Oct 29, 2024

@alonsofabila-dev I push another version with some minor changes:

  • Fix the another instance that you change %s to %q but did not remove the fmt.Sprintf
diff --git a/e2e/dractions/retry.go b/e2e/dractions/retry.go
index dddcd38e..cb4a285f 100644
--- a/e2e/dractions/retry.go
+++ b/e2e/dractions/retry.go
@@ -206,7 +206,7 @@ func waitDRPCDeleted(client client.Client, namespace string, name string) error
                }
 
                if time.Since(startTime) > time.Second*time.Duration(util.Timeout) {
-                       return fmt.Errorf(fmt.Sprintf("drpc %q is not deleted yet before timeout, fail", name))
+                       return fmt.Errorf("drpc %q is not deleted yet before timeout, fail", name)
                }
 
                time.Sleep(time.Second * time.Duration(util.TimeInterval))
  1. Fix the commit message to be in the standard format
  2. Fix the PR message by copying the commit message

@nirs
Copy link
Member

nirs commented Oct 29, 2024

@alonsofabila-dev git grep show other instances that you did not fix:

% git grep 'fmt.Errorf(fmt.Sprintf'
deployers/retry.go:                     return fmt.Errorf(fmt.Sprintf("subscription %s status is not %s yet before timeout", name, phase))
deployers/retry.go:                     return fmt.Errorf(fmt.Sprintf("workload %s is not ready yet before timeout of %v",
dractions/retry.go:                     return fmt.Errorf(fmt.Sprintf(
dractions/retry.go:                     return fmt.Errorf(fmt.Sprintf("drpc %s progression is not %s yet before timeout of %v",

@alonsofabila-dev alonsofabila-dev force-pushed the fix/Improper-use-of-error-formatting branch from 59a9b36 to aabe093 Compare October 29, 2024 17:47
@alonsofabila-dev
Copy link
Author

done

@nirs
Copy link
Member

nirs commented Oct 29, 2024

@alonsofabila-dev please rebase on main, some of the changes already fixed by another pr merged just now.

@nirs
Copy link
Member

nirs commented Oct 30, 2024

@alonsofabila-dev I asked to rebase on main, but you merged it - this is not the same. We want to have clean linear history in git without merge commits. The way to update your PR is to use:

git checkout main
git pull upstream main
git checkout your-pr-branch
git rebase main
(fix possible conflicts)
git push -f

This should be done each time you push changes, to make sure your change is tested on top of latest version.

@alonsofabila-dev alonsofabila-dev force-pushed the fix/Improper-use-of-error-formatting branch from 554078d to e949660 Compare October 30, 2024 16:41
@alonsofabila-dev
Copy link
Author

alonsofabila-dev commented Oct 30, 2024

sorry for my mistake, I have now done as you mentioned. but keep sayin that there are conflicts, and when i rebase on main says that its up to date, even though i see they are not the same

e2e: Fix improper use of formatting

When using fmt.Errorf(), we don't need to format the message with
fmt.Sprintf().

Fixes: RamenDR#1618
Signed-off-by: alonso fabila <[email protected]>
@nirs nirs force-pushed the fix/Improper-use-of-error-formatting branch from e949660 to 57ad0f9 Compare October 30, 2024 22:55
@nirs
Copy link
Member

nirs commented Oct 30, 2024

@alonsofabila-dev You rebased, but you had to "pick" only your commit. I clean up and resolved the conflicts.

@alonsofabila-dev
Copy link
Author

thanks

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.

e2e: Improper use of error formatting
2 participants