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

Migrate all commands to cobra style commands #472

Open
ahrtr opened this issue Apr 26, 2023 · 26 comments
Open

Migrate all commands to cobra style commands #472

ahrtr opened this issue Apr 26, 2023 · 26 comments

Comments

@ahrtr
Copy link
Member

ahrtr commented Apr 26, 2023

We have already implemented part of the surgery command as cobra style commands,

surgeryCmd := &cobra.Command{

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

  • bench
  • buckets
  • check
  • compact
  • dump
  • get
  • info
  • keys
  • page
  • pages
  • page-item
  • stats
  • surgery (@ahrtr will take care of the rest of surgery 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
Copy link
Member

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
Copy link
Member Author

ahrtr commented Apr 26, 2023

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.

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

What edge cases?

@serathius
Copy link
Member

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
Copy link
Member Author

ahrtr commented Apr 26, 2023

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
Copy link
Member Author

ahrtr commented Apr 26, 2023

Just added test cases to cover all the commands which do not have cases before. #474

@ahrtr ahrtr removed the help wanted label Apr 27, 2023
@ahrtr
Copy link
Member Author

ahrtr commented Apr 27, 2023

The plan is to migrate all surgery commands to cobra style commands firstly. Please take a look at the first PR #473

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

@serathius
Copy link
Member

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
Copy link
Member Author

ahrtr commented May 5, 2023

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
Copy link
Contributor

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
Copy link
Contributor

/cc @jmhbnz

@jmhbnz
Copy link
Member

jmhbnz commented Jun 16, 2023

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
Copy link
Member Author

ahrtr commented Jun 20, 2023

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 #472 (comment), migrating other commands to cobra style commands is a breaking change, so we decided to do it in 2.0.

@charles-chenzz
Copy link
Contributor

Thanks for the help. As I mentioned in #472 (comment), 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
Copy link
Contributor

ishan16696 commented Aug 11, 2023

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.

  • bench
  • buckets
  • check
  • compact
  • dump
  • get
  • info
  • keys
  • page
  • pages
  • page-item
  • stats
  • version (done by @ishan16696)
  • surgery (done by @ahrtr)

@Elbehery
Copy link
Member

Elbehery commented Nov 9, 2023

@ahrtr shall i take this on ?

@charles-chenzz
Copy link
Contributor

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

@github-actions github-actions bot added the stale label Apr 17, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 9, 2024
@ahrtr ahrtr reopened this May 9, 2024
@ahrtr ahrtr removed the stale label May 9, 2024
@Elbehery
Copy link
Member

Elbehery commented May 9, 2024

@ahrtr i can take this 👍🏽

@tommyshem
Copy link

I can do the the info command next when the buckets command above is ok to merge

tommyshem pushed a commit to tommyshem/bbolt that referenced this issue Nov 12, 2024
@Elbehery
Copy link
Member

@ahrtr hello

I thought we are holding this on, due to breaking changes, or ?

@ahrtr
Copy link
Member Author

ahrtr commented Nov 13, 2024

@ahrtr hello

I thought we are holding this on, due to breaking changes, or ?

As long as there isn't any behavior change from users' perspective, then it's OK to get it done in 1.4.

@ivanvc
Copy link
Member

ivanvc commented Nov 13, 2024

Just confirming that we're not considering a behavior change that Go flags are not POSIX compliant (single dash) and that it's okay to replace them with pflags (double dash/POSIX compliant)?

If we're okay with this change, I think we can start splitting the work. I originally migrated bench, but now it's outdated because of performance improvements and the addition of random read capabilities. So, I'd like to be assigned to bench.

@ahrtr
Copy link
Member Author

ahrtr commented Nov 13, 2024

Just confirming that we're not considering a behavior change that Go flags are not POSIX compliant (single dash) and that it's okay to replace them with pflags (double dash/POSIX compliant)?

single dash to double dash should be a breaking change.

tommyshem pushed a commit to tommyshem/bbolt that referenced this issue Nov 13, 2024
@tommyshem
Copy link

the buckets command has no flags only the path argument

@tommyshem
Copy link

PS C:\tmp\fork\bbolt1\cmd\bbolt> .\bbolt.exe page -h
usage: bolt page PATH pageid [pageid...]
or: bolt page --all PATH

Additional options include:

    --all
            prints all pages (only skips pages that were considered successful overflow pages)
    --format-value=auto|ascii-encoded|hex|bytes|redacted (default: auto)
            prints values (on the leaf page) using the given format.

Page prints one or more pages in human readable format.

the above is the output from the page command .
It's from the original code and it already uses double dash flags e.g. --all flag

@Elbehery
Copy link
Member

Just confirming that we're not considering a behavior change that Go flags are not POSIX compliant (single dash) and that it's okay to replace them with pflags (double dash/POSIX compliant)?

If we're okay with this change, I think we can start splitting the work. I originally migrated bench, but now it's outdated because of performance improvements and the addition of random read capabilities. So, I'd like to be assigned to bench.

+1, we can split the required work here, feel free to assign me on this 👍🏽

@ivanvc
Copy link
Member

ivanvc commented Nov 18, 2024

@Elbehery, I think a good starting point would be checking which commands don't have flags. These would be good candidates to be migrated without breaking changes.

tommyshem pushed a commit to tommyshem/bbolt that referenced this issue Nov 18, 2024
tommyshem pushed a commit to tommyshem/bbolt that referenced this issue Nov 22, 2024
@github-actions github-actions bot removed the stale label Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

9 participants