search

etcd-io / bbolt

An embedded key/value database for Go.
https://go.etcd.io/bbolt
MIT License
8.11k stars 631 forks source link

Migrate all commands to cobra style commands #472

Open ahrtr opened 1 year ago

ahrtr commented 1 year ago

We have already implemented part of the surgery command as cobra style commands, https://github.com/etcd-io/bbolt/blob/bd7d6e9f18bc139836feccb576339fbb8254fb41/cmd/bbolt/command_surgery_cobra.go#L28

We need to migrate all other commands to cobra style commands,

We can create a separate file for each command, such as command_page.go for the bbolt page command. Please anyone feel free to drop message if you are interested in migrating any command.

I will provide an example PR soon.

serathius commented 1 year ago

Idea seems right to me, however from what I see https://github.com/etcd-io/bbolt/pull/473/files makes a breaking change on command line from bolt surgery revert-meta-page SRC DST to revert-meta-page <bbolt-file> [options] where --output is an optional flag.

I think this is ok for new commands like surgery, however what's your plan for old commands. Do you plan breaking changes here? If not, how sure we are in our tests to prevent missing any edge cases?

ahrtr commented 1 year ago

Do you plan breaking changes here?

For bbolt golang public API, we will keep them compatible in all 1.x releases.

For commands, we have two options,

If not, how sure we are in our tests to prevent missing any edge cases?

What edge cases?

serathius commented 1 year ago

cc @ptabor

What edge cases?

Just making a point that before refactoring the codebase we should double check out test coverage for command line surface.

ahrtr commented 1 year ago

Just making a point that before refactoring the codebase we should double check out test coverage for command line surface.

It's a good suggestion. Currently we have test cases for most of the commands. Only three commands (check, page and page-item) do not have test cases. We can add test cases separately. Note that the surgery commands (which are not migrated yet) are in a separated & dedicated file, they are almost isolated with other old commands.

ahrtr commented 1 year ago

Just added test cases to cover all the commands which do not have cases before. https://github.com/etcd-io/bbolt/pull/474

ahrtr commented 1 year ago

The plan is to migrate all surgery commands to cobra style commands firstly. Please take a look at the first PR https://github.com/etcd-io/bbolt/pull/473

Afterwards, we can discuss whether to migrate other old commands. Should we keep it compatible for all old commands?

serathius commented 1 year ago

Do you plan breaking changes here?

For bbolt golang public API, we will keep them compatible in all 1.x releases.

For commands, we have two options,

    1. Migrate all new commands (e.g. surgery) to surgery style commands in master (1.4.0), and migrate all old commands in 2.0+;
    1. Migrate all commands to cobra style commands, as It isn't good to have mix command styles. Yes, it's breaking change.
    • If users want to keep using old style commands, they just need to use 1.3.6 or 1.3.7 commands even they bump the bbolt to 1.4.0;
    • Given the long release cycle, it might be accepted.

Based on my experience in K8s community I think maintaining backward compatibility is very important. My preference would be to do a proper migration similar to what we did for etcdctl. By default keep the old behavior, and allow users to switch to cobra style arguments by passing environment variable. However this will be time consuming I leave it up to you to decide if you can invest the time.

ahrtr commented 1 year ago

The plan is to migrate all surgery commands to cobra style commands in 1.4, and migrate all other old commands in 2.0. Does it make sense?

By default keep the old behavior, and allow users to switch to cobra style arguments by passing environment variable

It's a good suggestion in general; but I really don't want to complicate the bbolt CLI, and don't want to spend too much time on it.

charles-chenzz commented 1 year ago

We can create a separate file for each command, such as command_page.go for the bbolt page command. Please anyone feel free to drop message if you are interested in migrating any command.

I will provide an example PR soon

can you link example PR or are there refs? would like to help migrating part of the command @ahrtr

charles-chenzz commented 1 year ago

/cc @jmhbnz

jmhbnz commented 1 year ago

Hey @charles-chenzz - Please refer to https://github.com/etcd-io/bbolt/pulls?q=is%3Apr+migrate+cobra+style for examples of pull requests migrating commands to cobra style.

ahrtr commented 1 year ago

can you link example PR or are there refs? would like to help migrating part of the command @ahrtr

Thanks for the help. As I mentioned in https://github.com/etcd-io/bbolt/issues/472#issuecomment-1536207021, migrating other commands to cobra style commands is a breaking change, so we decided to do it in 2.0.

charles-chenzz commented 1 year ago

Thanks for the help. As I mentioned in https://github.com/etcd-io/bbolt/issues/472#issuecomment-1536207021, migrating other commands to cobra style commands is a breaking change, so we decided to do it in 2.0.

we slowing migrate parts of them from 1.4 - 2.0 or we plan to start the other migrate in one shot in 2.0? I already have a PR wip. If we do in one shot then I need to hold the PR

ishan16696 commented 1 year ago

Hi @charles-chenzz ,

I already have a PR wip. If we do in one shot then I need to hold the PR

are you working on all of bbolt commands ? I have created checkboxes to track progress and assignees for each bbolt commands.

Elbehery commented 10 months ago

@ahrtr shall i take this on ?

charles-chenzz commented 10 months ago

hi @ishan16696 , I was working on part of it. please feel free to take on.

Elbehery commented 4 months ago

@ahrtr i can take this 👍🏽