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

[#253] Refactor commands in pgagroal-cli #289

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

fluca1978
Copy link
Collaborator

Now pgagroal-cli has a set of "logically" grouped commands and
subcommands. For example, all the commands related to shutting down
the pooler are under the shutdown command, that can operate with
subcommands like gracefully, immediate or cancel.

In order to provide this capability, two new functions have been
introduced:

  • parse_command() accepts the command line and seek for a command,
    possibly its subcommand, and an optional "value" (often the database
    or server name).
  • parse_deprecated_command() does pretty much the same thing but
    against the old command. Thanks to this, old commands can still work
    and the user will be warned about their deprecation, but the interface
    of pgagroal-cli is not broken.

Both functions requires to know the offset at which start seeking for
a command, and that depends on the number of options already parsed
via getopt_long(). Since the &option_index is valued only for long
options, I decided to use the optind global value, see
getopt_long(3).
This value is initialized with the "next thing" to seek on the command
line, i.e., the next index on argv.

In the case the command accepts an optional database name, the
database value is automatically set to '' (all databases) in case the
database name is not found on the command line.
Therefore:
pgagroal-cli flush idle
is equivalent to
pgagroal-cli flush idle '
'

On the other hand, commands that require a server name get the value
automatically set to "\0" (an invalid server name) in order to "block"
other pieces of code. Moroever, if the server has not been specified,
the command is automatically set to "unknown" so that the help screen
is shown.

The pgagroal-cli has a set of pgagroal_log_trace() calls whenever
a command is "parsed", so that it is possible to quickly follow the
command line parsing.

Also, since the pgagroal-cli exists if no command line arguments
have been specified, the safety check aboutt argc > 0 around the
command line parsing have been removed.

In the case the user specified an unknown command, she is warned on
stdout before printing the usage() help screen.

Deprecated commands are notified to the user via a warning message,
printed on stderr, that provides some hints about the correct usage of
the new command.

Documentation has been updated and improved.
A specific documentation section about deprecated
commands have been introduced.

Close #253

@fluca1978
Copy link
Collaborator Author

In the case this is a good implementation, we could refactor similarly also pgagroal-admin too.

@jesperpedersen
Copy link
Collaborator

We should keep the reset command for now, and I don't think we should display the deprecated message in the 1.x branches.

The rest looks good, and changing pgagroal-admin as well would be nice. Another thing could be shell completion based on the new commands.

@fluca1978
Copy link
Collaborator Author

We should keep the reset command for now

The problem with having reset is that we will not be able to "catch" the usage of the deprecated command in place of the new one. That's why I changed the word into clear. Any suggestion here is welcome.

, and I don't think we should display the deprecated message in the 1.x branches.

I can add a check into parse_deprecated_command to emit the warning only if pgagroal version is greater than a constant.

The rest looks good, and changing pgagroal-admin as well would be nice.

I will open an issue about that and start working on it, but I need to move the utility functions into a shared file (i.e, making them not static), therefore that is going to influence also this implementation.

Another thing could be shell completion based on the new commands.

Yeah, but I've no idea at the moment on how to do that.

fluca1978 added a commit to fluca1978/pgagroal that referenced this pull request Jul 28, 2022
This commit introduces two compile time macros:
- PGAGROAL_VERSION is a string and contains the application version
that will be shown in various application messages.
- PGAGROAL_VERSION_NUM is a numeric value that can be used to compare
the running version with another number, so for example to limit
features or messages.

`PGAGROAL_VERSION` replaces the `VERSION` macro that was defined in
the main header file.

Moreover, the message printed at level INFO when booting the pooler
has been enhanced to show also the version that is starting, like for
instance:
    pgagroal: v1.5.0 started on *:5432

See agroal#253, agroal#289, agroal#290

Close agroal#292
@jesperpedersen
Copy link
Collaborator

Yes, we can use your VERSION pull request for this; set to PGAGRAOL_VERSION_MAJOR to 2

fluca1978 added a commit to fluca1978/pgagroal that referenced this pull request Jul 28, 2022
Three new constants are introduced here:
- PGAGROAL_MAJOR_VERSION (number)
- PGAGROAL_MINOR_VERSION (number)
- PGAGROAL_PATCH_VERSION (number)
- PGAGROAL_VERSION       (string)

that represent, as integer literals and string composition,
the parts of the version number.
The values are copied from the variables in the `CmakeLists.txt` file
that creates the building flow, so changing the values in a single
place changes and propagates the changes all over the source code.

A few utility functions have been introduced to compose the three
constant into a unique number that can be used to compare the version
currently running, so that features and/or message can be targeted as
conditionals.

Introduced functions are:
- pgagroal_version_as_number() returns an unique sortable version
number. For example, version 1.5.0 is returned as 15000, version 1.6.2
as 16002, and so on;
- pgagroal_version_number() returns the currently running version
number, that is invokes pgagroal_version_as_number() with the
predefined constants.

The `pgagroal_version_as_number()` accepts individual values, while
`pgagroal_version_number()` provides the currently running version
number. The idea is that, thanks to both the functions, it is possible
to check for a feature that depends on a version in a way like the
following one:

   if (pgagroal_version_numer() >=
             pgagroal_version_as_number(2,1,0))
       // version is greater than 2.1.0 !

The utility function `pgagroal_version_ge()` does exactly the above:
it accepts the three parts of a version number and checks if the
currently running version is greater or equal (hence, 'ge') of the
specified triplet, so that the above piece of code becomes:

   if (pgagroal_version_ge(2,1,0))
     // version greater or equal 2.1.0

Close agroal#292
See agroal#253, agroal#289, agroal#290, agroal#293
fluca1978 added a commit to fluca1978/pgagroal that referenced this pull request Jul 28, 2022
Three new constants are introduced here:
- PGAGROAL_MAJOR_VERSION (number)
- PGAGROAL_MINOR_VERSION (number)
- PGAGROAL_PATCH_VERSION (number)
- PGAGROAL_VERSION       (string)

that represent, as integer literals and string composition,
the parts of the version number.
The values are copied from the variables in the `CmakeLists.txt` file
that creates the building flow, so changing the values in a single
place changes and propagates the changes all over the source code.

A few utility functions have been introduced to compose the three
constant into a unique number that can be used to compare the version
currently running, so that features and/or message can be targeted as
conditionals.

Introduced functions are:
- pgagroal_version_as_number() returns an unique sortable version
number. For example, version 1.5.0 is returned as 15000, version 1.6.2
as 16002, and so on;
- pgagroal_version_number() returns the currently running version
number, that is invokes pgagroal_version_as_number() with the
predefined constants.

The `pgagroal_version_as_number()` accepts individual values, while
`pgagroal_version_number()` provides the currently running version
number. The idea is that, thanks to both the functions, it is possible
to check for a feature that depends on a version in a way like the
following one:

   if (pgagroal_version_numer() >=
             pgagroal_version_as_number(2,1,0))
       // version is greater than 2.1.0 !

The utility function `pgagroal_version_ge()` does exactly the above:
it accepts the three parts of a version number and checks if the
currently running version is greater or equal (hence, 'ge') of the
specified triplet, so that the above piece of code becomes:

   if (pgagroal_version_ge(2,1,0))
     // version greater or equal 2.1.0

Close agroal#292
See agroal#253, agroal#289, agroal#290, agroal#293
@fluca1978
Copy link
Collaborator Author

I think I've a complete piece of code, but I'm postponing this when the new version will be released.

@jesperpedersen
Copy link
Collaborator

Lets hold this for 1.6

@fluca1978
Copy link
Collaborator Author

Commit 53884cf is another attempt at get the command refactoring done.
Please take into account that this changes the documentation, so we should keep it separated by the currently available documentation on the repository by mean of a dedicated branch (e.g., development).

@jesperpedersen
Copy link
Collaborator

Lets promote the new CLI commands, but allow the old ones.

Since we are not in 2.x mode; there should not be a warning using the "old" commands.

Could you rebase ?

@fluca1978
Copy link
Collaborator Author

I've rebased so far, but at this point I would wait for #329 to be implemented (and I don't know when it will happen, since it is quite challenging).

With regard to the warning, right now it is set to be placed when the version is less than 1.6 (not yet released), we can pump it to 2 so that the warning is already there but not shown before version 2 is released.

However, one problem I see is that the documentation changes accordingly, so we risk to have an online documentation that does not reflect the current stable release.

@jesperpedersen
Copy link
Collaborator

Ok, lets wait for config-set.

I think we should change the documentation to the new commands - the old commands will be in place for existing installations.

…groal-admin`

Now `pgagroal-cli` has a set of "logically" grouped commands and
subcommands. For example, all the commands related to shutting down
the pooler are under the `shutdown` command, that can operate with
subcommands like `gracefully`, `immediate` or `cancel`.

In order to provide this capability, new functions have been
introduced as utilities:
- `parse_command()` accepts the command line and seek for a command,
possibly its subcommand, and an optional "value" (often the database
or server name).
- `parse_command_simple()` is a wrapper around the above
`parse_command` that shorten the function call line because it does
not require to specify the key and the value (and their defaults).
- `parse_deprecated_command()` does pretty much the same thing but
against the old command. Thanks to this, old commands can still work
and the user will be warned about their deprecation, but the interface
of `pgagroal-cli` is not broken.

All the above functions require to know the offset at which start seeking for
a command, and that depends on the number of options already parsed
via `getopt_long()`. Since the `&option_index` is valued only for long
options, I decided to use the `optind` global value, see
getopt_long(3).
This value is initialized with the "next thing" to seek on the command
line, i.e., the next index on `argv`.

In the case the command accepts an optional database name, the
database value is automatically set to '*' (all databases) in case the
database name is not found on the command line.
Therefore:
   pgagroal-cli flush idle
is equivalent to
   pgagroal-cli flush idle '*'

On the other hand, commands that require a server name get the value
automatically set to "\0" (an invalid server name) in order to "block"
other pieces of code. Moroever, if the server has not been specified,
the command is automatically set to "unknown" so that the help screen
is shown.

The `pgagroal-cli` has a set of `pgagroal_log_debug()` calls whenever
a command is "parsed", so that it is possible to quickly follow the
command line parsing.

Also, since the `pgagroal-cli` exists if no command line arguments
have been specified, the safety check aboutt `argc > 0` around the
command line parsing has been removed.

In the case the user specified an unknown command, she is warned on
stdout before printing the `usage()` help screen.

Deprecated commands are notified to the user via a warning message,
printed on stderr, that provides some hints about the correct usage of
the new command. The warning about deprecated commands is shown only
if the currently running version of the software is greater than the
version the command has been deprecated onto. In particular these
commands have been deprecated since 1.6.

This commit also introduces the command refactoring for `pgagroal-admin` in
a way similar to the work done for `pgagroal-cli`.
New commands are available:
- user <what>
with <what> being <add>, <del>, <edit>, <ls>.

Updated:
- documentation
- shell completions
- help screens
- examples

Close agroal#290 agroal#253
@fluca1978
Copy link
Collaborator Author

@jesperpedersen can you please give ade4024 a try? I think this is complete now.

@jesperpedersen
Copy link
Collaborator

Cool, I'll give this a review next week

@jesperpedersen jesperpedersen merged commit ade4024 into agroal:master Oct 13, 2023
2 checks passed
@jesperpedersen
Copy link
Collaborator

Merged.

Thanks for your contribution !

We can consider to just remove the deprecated commands... but that is a discussion in https://github.com/agroal/pgagroal/discussions/categories/ideas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor command names in pgagroal-cli
2 participants