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

[cmd] Enable db-manager CLI to support subcommands #1122

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

mickmis
Copy link
Contributor

@mickmis mickmis commented Sep 20, 2024

This PR refactors the db-manager command to support subcommands (notably for #1116).

  • The existing db-manager logic is now defined as the migration subcommand. Various updates of scripts, deployment files, and documentations are made in that regard.
  • Introduce use of spf13/cobra and spf13/pflag libraries to support this.
  • Note that not all flags are migrated to using spf13/pflag, however the existing ones that use flag are still supported (notably for the cockroachdb flags).
  • go mod tidy was ran.
  • The diff is unfortunately not very readable, however here is a change overview that should help review:
    • cmds/db-manager/main.go is a new file
    • cmds/db-manager/main.go was moved to cmds/db-manager/migration/migration.go and adapted:
      • panics are replaced by returning errors
      • flags are defined with spf13/pflag
      • context is passed from parent
      • check for parameter schemas_dir is replaced by use of function MarkFlagRequired

Copy link
Member

@BenjaminPelletier BenjaminPelletier left a comment

Choose a reason for hiding this comment

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

It is annoying when git doesn't recognize a move + edit. When that is known to be coming, the move can be done in the first commit (updating references, etc as necessary) and then the changes can be done in a second commit so reviewers can look at the changes separately from the move. But not worth doing for this PR, I think.

cmds/db-manager/main.go Outdated Show resolved Hide resolved
cmds/core-service/README.md Outdated Show resolved Hide resolved
@mickmis
Copy link
Contributor Author

mickmis commented Sep 24, 2024

It is annoying when git doesn't recognize a move + edit. When that is known to be coming, the move can be done in the first commit (updating references, etc as necessary) and then the changes can be done in a second commit so reviewers can look at the changes separately from the move. But not worth doing for this PR, I think.

Indeed, I've not thought about it, thanks for the tip. Ended up doing it in this PR it was fast enough.

@mickmis mickmis merged commit 8ee12cd into interuss:master Sep 24, 2024
6 checks passed
@mickmis mickmis deleted the 1074/cli-db-manager branch September 24, 2024 10:40
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.

2 participants