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.

Sign up for GitHub

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

Jump to bottom

command buckets converted to the cobra style #859

Closed
wants to merge 2 commits into from
Closed

command buckets converted to the cobra style #859

wants to merge 2 commits into from

Conversation

tommyshem
Copy link

issue #472 Migrate all commands to cobra style commands

converted buckets command to cobra style

@k8s-ci-robot
Copy link

Hi @tommyshem. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@ivanvc
Copy link
Member

ivanvc commented Nov 11, 2024

Hi @tommyshem, thanks for your pull request. Could you please sign your commit? So the developer certificate of origin (DCO) check passes, i.e:

git rebase HEAD~1 --signoff
git push --force

Thanks!

Ref: https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#commit-your-change

/ok-to-test

@ahrtr
Copy link
Member

ahrtr commented Nov 11, 2024

Thanks for raising the PR. It doesn't break the user experience, so I agree to get this PR included in 1.4.

Please resolve the test failure in case TestBucketsCommand_Run.

tommyshem
tommyshem commented Nov 11, 2024
View reviewed changes
cmd/bbolt/main_test.go Outdated Show resolved Hide resolved
@ahrtr
Copy link
Member

ahrtr commented Nov 12, 2024

Please signoff the commit, read https://github.com/etcd-io/bbolt/pull/859/checks?check_run_id=32833799850

tommyshem
tommyshem commented Nov 12, 2024
View reviewed changes
cmd/bbolt/bbolt.exe Outdated Show resolved Hide resolved
@tommyshem
Copy link
Author

Should be finished now for review.
But some how I upload the bbolt.exe in the pull request and I don't know how to remove it.

ahrtr
ahrtr reviewed Nov 13, 2024
View reviewed changes
cmd/bbolt/command_version.go Outdated Show resolved Hide resolved
@tommyshem
Copy link
Author

Removed the bolt.exe file and the command_version.go file.
So it should be just the buckets command change now hopefully.

ahrtr
ahrtr reviewed Nov 14, 2024
View reviewed changes
cmd/bbolt/command_buckets.go Outdated Show resolved Hide resolved
cmd/bbolt/command_buckets.go Outdated Show resolved Hide resolved
cmd/bbolt/command_buckets.go Show resolved Hide resolved
cmd/bbolt/command_buckets_test.go Outdated Show resolved Hide resolved
@tommyshem
Copy link
Author

All the changes from above should be done now.

@ahrtr
Copy link
Member

ahrtr commented Nov 19, 2024

@tommyshem you need to rebase this PR instead of merging main branch into your dev branch. Please also squash the commits into one.

@tommyshem
Copy link
Author

Sorry new to github pull requests as only used github mainly for personal projects. I will try tomorrow to rebase and squash commits. Do I need to put any info in the commit text or just reference the issue number.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tommyshem
Once this PR has been reviewed and has the lgtm label, please assign serathius for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ahrtr
ahrtr reviewed Nov 22, 2024
View reviewed changes
cmd/bbolt/command_buckets.go Outdated
Comment on lines 24 to 25
if srcDBPath == "" {
return ErrPathRequired
Copy link
Member

Choose a reason for hiding this comment

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

I think the cobra.ExactArgs(1) already guarantees that it won't be empty string? Pleasae follow the same pattern as other command.

Copy link
Author

Choose a reason for hiding this comment

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

This check is from the original code.

Copy link
Member

Choose a reason for hiding this comment

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

"the original code" doesn't mean that it's necessary.

Please also always remember to simplify the the implementation,

bbolt/cmd/bbolt/command_inspect.go

Lines 26 to 30 in cb0618b

func inspectFunc(srcDBPath string) error {
if _, err := checkSourceDBPath(srcDBPath); err != nil {
return err
}

Copy link
Member

Choose a reason for hiding this comment

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

In order to avoid long back-and-forth, please see #859 (comment)

cmd/bbolt/command_buckets.go
// Print the list of buckets in the database.
return db.View(func(tx *bolt.Tx) error {
return tx.ForEach(func(name []byte, _ *bolt.Bucket) error {
fmt.Fprintln(cmd.OutOrStdout(), string(name))
Copy link
Member

Choose a reason for hiding this comment

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

I agree that cmd.OutOrStdout() is a little better than os.Stdout, but please keep it consistent with other commands. We can replace them with cmd.OutOrStdout() in separate PR.

Copy link
Author

Choose a reason for hiding this comment

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

cmd.OutOrStdout() is all ready used in the cobra style converted files already
command_check.go line 44, 65, 70
command_get.go line 46, 92
command_info.go line 47
command_version.go line 18, 19, 20

Copy link
Member

Choose a reason for hiding this comment

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

I see that it's only used in command_check.go. I should have raised this comment in PR 723.

Overall, not a big deal. Feel free to keep it as it's

Copy link
Author

Choose a reason for hiding this comment

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

I must have changed the over files in my copy above when I was testing the outputs.
As I had trouble capturing the output when I had converted to the cobra style without the cmd.OutOrStdout() function

ahrtr
ahrtr reviewed Nov 25, 2024
View reviewed changes
cmd/bbolt/command_buckets.go Outdated Show resolved Hide resolved
@tommyshem @ahrtr
remove srcDBPath check
588bfbb 
Co-authored-by: Benjamin Wang <[email protected]>
@tommyshem tommyshem closed this Dec 6, 2024
@ahrtr
Copy link
Member

ahrtr commented Dec 6, 2024

This PR is almost ready to be approved & merged.

Please squash the commits and ensure it's signed off. thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahrtr ahrtr ahrtr left review comments

@ivanvc ivanvc ivanvc left review comments

Assignees
No one assigned
Labels
ok-to-test size/L
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tommyshem @k8s-ci-robot @ivanvc @ahrtr