Skip to content
This repository was archived by the owner on Jul 24, 2024. It is now read-only.

cmd,task: disable stray command line args, and log the command in LogArguments#579

Merged
overvenus merged 5 commits into
pingcap:masterfrom
kennytm:cmdline-improvements
Nov 6, 2020
Merged

cmd,task: disable stray command line args, and log the command in LogArguments#579
overvenus merged 5 commits into
pingcap:masterfrom
kennytm:cmdline-improvements

Conversation

@kennytm
Copy link
Copy Markdown
Collaborator

@kennytm kennytm commented Nov 3, 2020

What problem does this PR solve?

  1. It was confusing that --checksum false does not actually disable checksum, because --checksum as a boolean flag already means --checksum=true. So --checksum false becomes --checksum=true false, and that extra false becomes a stray argument, which is ignored before this PR.

  2. LogArguments did not record whether it is a "backup" or "restore" making it a bit hard to determine the data direction when reading logs.

What is changed and how it works?

  1. Added Args: cobra.NoArgs to all leaf commands so using --checksum false will result in

    Error: unknown command "false" for "br backup full"

    rather than silently keeping checksum.

  2. Record the command being executed inside LogArguments.

    Note that I changed LogArguments from using VisitAll to Visit. Originally in add log for cmd arguments and duration for checksum #55 it was using VisitAll to print all args, however it was changed in Don't log secret of s3. #292 to condition on f.Changed, so perhaps let's just use Visit directly 🙃.

Check List

Tests

  • Manual test (add detailed scripts or steps below)
    • Run and check the log.

Code changes

Side effects

Related changes

  • Need to cherry-pick to the release branch

Release Note

  • BR no longer accepts --checksum false in command line, which did not disable checksum. The correct usage always include the = sign: --checksum=false.

overvenus
overvenus approved these changes Nov 3, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 LGTM1 label Nov 3, 2020
@overvenus
Copy link
Copy Markdown
Member

overvenus commented Nov 3, 2020

Please update your PR title.https://github.com/pingcap/community/blob/01457b0bba90b92a6d8f6c715a3b18cc432c6a12/contributors/commit-message-pr-style.md

@kennytm kennytm changed the title disable stray command line args, and log the command in LogArguments cmd,task: disable stray command line args, and log the command in LogArguments Nov 3, 2020
@overvenus
Copy link
Copy Markdown
Member

overvenus commented Nov 4, 2020

/run-integration-tests

1 similar comment
@overvenus
Copy link
Copy Markdown
Member

overvenus commented Nov 4, 2020

/run-integration-tests

@kennytm
Copy link
Copy Markdown
Collaborator Author

kennytm commented Nov 5, 2020

/rebuild

@overvenus
Copy link
Copy Markdown
Member

overvenus commented Nov 6, 2020

/run-integration-tests

Tests fail due to recent change in TiKV which has been fixed by tikv/tikv#8971

@kennytm
Copy link
Copy Markdown
Collaborator Author

kennytm commented Nov 6, 2020

/run-integration-test

https://internal.pingcap.net/idc-jenkins/blue/organizations/jenkins/br_ghpr_unit_and_integration_test/detail/br_ghpr_unit_and_integration_test/3629/pipeline/77 (unit test) 🤔

[2020-11-06T06:34:26.479Z] CGO_ENABLED=0 tools/bin/golangci-lint run --enable-all --deadline 120s \
...
[2020-11-06T06:36:11.482Z] make[1]: *** [static] Killed

https://internal.pingcap.net/idc-jenkins/blue/organizations/jenkins/br_ghpr_unit_and_integration_test/detail/br_ghpr_unit_and_integration_test/3629/pipeline/112 (br_tiflash)

[2020-11-06T06:39:28.878Z] failed to restore, before: 1000; after: 960

@overvenus overvenus merged commit 8d9a51a into pingcap:master Nov 6, 2020
@ti-srebot ti-srebot mentioned this pull request Nov 6, 2020
Merged
@ti-srebot
Copy link
Copy Markdown
Contributor

ti-srebot commented Nov 6, 2020

cherry pick to release-4.0 in PR #588

overvenus pushed a commit that referenced this pull request Nov 10, 2020
@ti-srebot @kennytm
cmd,task: disable stray command line args, and log the command in Log…
2ac49a4 
…Arguments (#579) (#588)

* cmd: disallow stray args

* task: in LogArguments, log the command name too

Co-authored-by: kennytm <kennytm@gmail.com>
@kennytm kennytm deleted the cmdline-improvements branch December 17, 2020 09:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@overvenus overvenus overvenus approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

needs-cherry-pick-release-4.0 status/LGT1 LGTM1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kennytm @overvenus @ti-srebot