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

add a 'flags' subcommand to list global flags #1033

Merged
merged 1 commit into from Nov 19, 2021

Conversation

LeGEC
Copy link
Contributor

@LeGEC LeGEC commented Jul 6, 2021

Database name

I work with PostgreSQL, right now I only added this flags subcommand to main/pg

Pull request description

in main/pg binary :

add a 'flags' subcommand to list global flags

additionally :

  • remove this list from the default list of options,
  • change default doc template to mention :
    "list available flags with 'wal-g flags'"

fixes #1032 (for main/pg)

@LeGEC LeGEC requested a review from a team as a code owner July 6, 2021 16:07
@usernamedt usernamedt self-requested a review August 1, 2021 13:44
@usernamedt
Copy link
Member

usernamedt commented Aug 18, 2021

Hi! This is a cool idea. The amount of auto-generated flags is definitely overwhelming.

Currently, when I run the wal-g flags command with no configuration, I get the following error:

➜  pg git:(1032-pg-help-flags) ./wal-g flags                                                                                                                                                                   
ERROR: 2021/08/18 16:15:03.156199 Failed to find any configured storage

I guess that this command should not depend on any configuration parameters, but simply output the available flags.

This happens because of the AssertRequiredSettingsSet() call in the PersistentPreRun of the pg command. Maybe there is another way to add the flags subcommand?

Copy link
Member

@usernamedt usernamedt left a comment

Choose a reason for hiding this comment

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

needs changes

@LeGEC
Copy link
Contributor Author

LeGEC commented Aug 26, 2021

@usernamedt yes ; FWIW ./wal-g help has the same issue

@LeGEC LeGEC force-pushed the 1032-pg-help-flags branch 3 times, most recently from c555240 to b7f3020 Compare October 17, 2021 10:26
@LeGEC
Copy link
Contributor Author

LeGEC commented Oct 18, 2021

update :

  • fix the linting error
  • commands 'help' and 'flags' don't need to have a configured storage, don't fail with "Failed to find any configured storage" error

cmd/pg/flags.go Outdated Show resolved Hide resolved
cmd/pg/flags.go Outdated Show resolved Hide resolved
@usernamedt
Copy link
Member

usernamedt commented Nov 16, 2021

Command itself looks good, but I think that flags should be a global subcommand for any database. Can you move it to a separate package, for example cmd/flags/flags.go? You can use storage tools global subcommand as a reference.

@usernamedt
Copy link
Member

I moved the flags subcommand and all common cmd init logic to the common. Now it should work fine for all databases.

Copy link
Contributor

@mialinx mialinx left a comment

Choose a reason for hiding this comment

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

LGTM

@usernamedt usernamedt merged commit 0841410 into wal-g:master Nov 19, 2021
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.

All help messages list the complete set of global flags
4 participants