feat(cli): implement personality for application name-agnostic help manuals #238

anpep · 2023-06-14T10:07:37Z

Implements the concept of personality, two variables that contain information about the application identity, such as the program binary name and the application display name. This enables the implementation of Pebble-based software that does not necessarily have the same name as Pebble.
Help strings throughout the application have been slightly modified so self-references to pebble/Pebble are removed without changing the semantics of the help manual.

* Implements the concept of personality, a struct that contains information about the application identity, such as the program binary name and the application display name. This enables the implementation of Pebble-based software that does not necessarily have the same name as Pebble. * Help strings throughout the application have been slightly modified so self-references to pebble/Pebble are removed without changing the semantics of the help manual.

atesburak · 2023-06-14T11:34:18Z

internals/cli/cmd_enter.go

@@ -160,7 +160,7 @@ func (cmd *cmdEnter) Execute(args []string) error {
 	case runStop = <-runReadyCh:
 	case runPanic := <-runResultCh:
 		if runPanic == nil {
-			panic("internal error: pebble daemon stopped early")
+			panic("internal error: daemon stopped early")


In case of multiple services wouldn't the program name be useful in a panic?

That's fine -- I'd be happy to reinstate it as {{.DisplayName}}.

internals/cli/cmd_exec.go

internals/cli/cmd_help.go

If the client panics due to the Pebble daemon stopping early, include the program name from personality so it's unambiguous.

flotter

Looks close! Few ideas to simplify mostly.

internals/cli/cli.go

internals/cli/cmd_enter.go

internals/cli/cmd_help.go

internals/cli/cmd_run.go

internals/cli/cmd_help.go

internals/cli/cli.go

internals/cli/cmd_help.go

internals/cli/cmd_run.go

internals/cli/cmd_help.go

atesburak

Looks on par up to last week's discussion

anpep · 2023-07-11T14:40:14Z

Actually, this is missing the logic for replacing <pebble> with something else from personality; however I think it would be a great starting point to merge this along with the CLI API and then implement the replacement logic, because it has to be done in the parser code which has been rewritten in the CLI API PR.

…

On Tue, Jul 11, 2023 at 16:38 burak ***@***.***> wrote: ***@***.**** approved this pull request. Looks on par with last week's discussion — Reply to this email directly, view it on GitHub <#238 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANEOQKP3V6DJTC3NW7YUNLLXPVQNBANCNFSM6AAAAAAZGDWF3I> . You are receiving this because you were assigned.Message ID: ***@***.***>

Argument, option and command descriptions can now utilize the `<display name>` and `<program name>` notation that will be substituted with the corresponding values depending on the current binary personality.

anpep · 2023-07-24T08:04:16Z

This PR is ready for review now, but changes will be required when #232 is merged.

This patch, similar in spirit to canonical#238, introduces a new public API for extending the Pebble client. The basic idea behind this patch is the client mixin struct embedded in commands' structs no longer holds an instance of the Pebble-specific `client.Client` struct, but instead take a `ClientGetter` interface that implements a `Client()` method. `ClientGetter.Client()` always returns a Pebble-specific `client.Client` struct. For applications that indent to extend Pebble, this means that either the Pebble client or a new, application-specific client can be used. In the latter case, the application-specific client must implement the `ClientGetter` interface so that a Pebble-specific `client.Client` struct can always be derived by the facilities consuming the `ClientGetter` interface. The easiest way to implement this is to embed the Pebble client: type PebbleClient = client.Client type MyClient struct { *PebbleClient } Since the Pebble-specific `client.Client` is embedded, and the `ClientGetter` interface requires `Client()` to be implemented with a pointer receiver, the snippet above suffices to implement a client based off the Pebble-supplied one without much hassle, and that provides lower-level facilities for communicating with the daemon, such as `DoSync()`, `DoAsync()` and `DoAsyncFull()`.

flotter

Looks good to me. (Just the one typo to fix)

internals/cli/cmd_run.go

paul-rodriguez

Seems ok

niemeyer

Thanks! Quite a few comments, but pretty simple stuff mostly:

internals/cli/cmd_help.go

internals/cli/cli.go

internals/cli/cmd_exec.go

internals/cli/cmd_help.go

internals/cli/cmd_run.go

internals/cli/cmd_warnings.go

cmd/version.go

* `PersonalityInfo` has been removed in favour of independent variables. * Change `<personality var>` syntax to `{{.PersonalityVar}}`. (e.g. `{{.ProgramName}}` instead of `<program name>`) * Fixed `manfixer` logic in the help command. * Restored variables in help command instead of inline strings.

anpep · 2023-11-15T08:49:43Z

@benhoyt What's the state of this? Do you think it's merge ready?

benhoyt

Some changes requested. Note also that the usage help still says "Usage: pebble ...". Can we change that? For example, I've changed ProgramName to elbbep locally, and get this:

$ go run ./cmd/pebble run -h
Usage:
  pebble run [run-OPTIONS]
...

cmd/version.go

internals/cli/cli.go

internals/cli/cmd_enter.go

benhoyt · 2023-11-15T22:31:09Z

internals/cli/cmd_help.go

 }

 func (w *manfixer) Write(buf []byte) (int, error) {
 	if !w.done {
 		w.done = true
-		if bytes.HasPrefix(buf, []byte(".TH pebble 1 ")) {
+		prefix := ".TH " + w.programName + " "


We probably had this discussion already ages ago, but can you please explain again why this changes from ".TH pebble 1 " to ".TH pebble ", and why the slicing lengths change below?

The fact that I don't recall the details about this issue validates your point even further (: I'll look at it, fail again to find a better way to do this, and really document it this time.

(Also: see canonical/go-flags#5)

So, apparently this is not changing from .TH pebble 1 to .TH pebble . What it's doing is identify the .TH <program name> prefix, find it, and replace the man section number afterwards by an 8 (instead of the default 1).

The slicing lengths change from fixed magic numbers to the actual lengths of the prefix so that the code appears to be less magic than it was before.

The w.programName bit comes from the actual go-flags understanding of what is the program name (cmd.parser.Name).

Thanks. If you could add an updated comment about what's going on here, that would be great.

internals/cli/cmd_help.go

benhoyt · 2023-11-15T22:32:49Z

internals/cli/cmd_run.go

@@ -31,16 +31,16 @@ import (
 	"github.com/canonical/pebble/internals/systemd"
 )

-const cmdRunSummary = "Run the pebble environment"
+const cmdRunSummary = "Run the {{.DisplayName}} environment"
 const cmdRunDescription = `
 The run command starts pebble and runs the configured environment.


Need to override this instance too? Probably with {{.DisplayName}}.

Looks like this comment was resolved but not done? The cmdRunDescription still says "The run command starts pebble..."

internals/cli/cmd_run.go

benhoyt · 2023-11-15T22:34:42Z

internals/cli/cmd_signal.go

@@ -28,7 +28,7 @@ const cmdSignalDescription = `
 The signal command sends a signal to one or more running services. The signal
 name must be uppercase, for example:

-pebble signal HUP mysql nginx
+	{{.ProgramName}} signal HUP mysql nginx


Any reason this should be indented?

It's indented everywhere else (: Also, the pattern is to even include the shell caret ($) which I'm not sure if I should include here as well for consistency.

The other example (in cmd_exec.go) doesn't use indentation or the $ prefix. How about we drop them here and in cmd_run.go for consistency?

$ rg '^\s*pebble ' -C2 cmd_signal.go 29-name must be uppercase, for example: 30- 31:pebble signal HUP mysql nginx 32-` 33- cmd_exec.go 40-arguments using "--", for example: 41- 42:pebble exec --timeout 10s -- echo -n foo bar 43-` 44-

anpep · 2023-11-20T08:04:00Z

Note also that the usage help still says "Usage: pebble ...". Can we change that?

I'll have to look if go-flags allows for such a change. Otherwise I'm afraid we might be stuck with it taking the binary name from argv0.

benhoyt · 2023-11-20T21:51:13Z

Note also that the usage help still says "Usage: pebble ...". Can we change that?

I'll have to look if go-flags allows for such a change. Otherwise I'm afraid we might be stuck with it taking the binary name from argv0.

Looks like we could change it with this tweak:

diff --git a/internals/cli/cli.go b/internals/cli/cli.go
index 4de25ac..86c46e8 100644
--- a/internals/cli/cli.go
+++ b/internals/cli/cli.go
@@ -166,6 +166,7 @@ func Parser(cli *client.Client) *flags.Parser {
 
        flagOpts := flags.Options(flags.PassDoubleDash)
        parser := flags.NewParser(&defaultOpts, flagOpts)
+       parser.Command.Name = cmd.ProgramName
        parser.ShortDescription = "System and service manager"
        parser.LongDescription = applyPersonality(longPebbleDescription)

benhoyt

Few more minor comments. See note about modifying the "Usage:" thing above.

benhoyt · 2023-11-20T21:32:50Z

internals/cli/cmd_signal.go

@@ -28,7 +28,7 @@ const cmdSignalDescription = `
 The signal command sends a signal to one or more running services. The signal
 name must be uppercase, for example:

-pebble signal HUP mysql nginx
+	{{.ProgramName}} signal HUP mysql nginx


The other example (in cmd_exec.go) doesn't use indentation or the $ prefix. How about we drop them here and in cmd_run.go for consistency?

$ rg '^\s*pebble ' -C2 cmd_signal.go 29-name must be uppercase, for example: 30- 31:pebble signal HUP mysql nginx 32-` 33- cmd_exec.go 40-arguments using "--", for example: 41- 42:pebble exec --timeout 10s -- echo -n foo bar 43-` 44-

run-checks

benhoyt · 2023-11-20T21:38:51Z

internals/cli/cmd_help.go

 }

 func (w *manfixer) Write(buf []byte) (int, error) {
 	if !w.done {
 		w.done = true
-		if bytes.HasPrefix(buf, []byte(".TH pebble 1 ")) {
+		prefix := ".TH " + w.programName + " "


Thanks. If you could add an updated comment about what's going on here, that would be great.

benhoyt · 2023-11-20T21:43:15Z

internals/cli/cmd_run.go

@@ -31,16 +31,16 @@ import (
 	"github.com/canonical/pebble/internals/systemd"
 )

-const cmdRunSummary = "Run the pebble environment"
+const cmdRunSummary = "Run the {{.DisplayName}} environment"
 const cmdRunDescription = `
 The run command starts pebble and runs the configured environment.


Looks like this comment was resolved but not done? The cmdRunDescription still says "The run command starts pebble..."

benhoyt · 2023-11-20T21:44:37Z

internals/cli/cmd_enter.go

@@ -160,7 +160,7 @@ func (cmd *cmdEnter) Execute(args []string) error {
 	case runStop = <-runReadyCh:
 	case runPanic := <-runResultCh:
 		if runPanic == nil {
-			panic("internal error: pebble daemon stopped early")
+			panic("internal error: daemon stopped early")


That's fine -- I'd be happy to reinstate it as {{.DisplayName}}.

benhoyt · 2023-11-20T22:37:50Z

One more thing. Now that the pebble notices command is merged, we need to update that to use {{.ProgramName}} too. I've changed ProgramName and DisplayName locally to something other than "pebble" / "Pebble", and get this (the run one I've already noted above):

$ go run ./cmd/pebble help --man >manpage 
$ grep -i pebble manpage
may then be acknowledged by running 'pebble okay'. When a notice repeats, it
The \fIrun\fP command starts pebble and runs the configured environment.

internals/cli/cmd_warnings.go

paul-rodriguez

run-checks should be removed. I was confused by one of the changes putting back an explicit "pebble".

benhoyt

Looks good to me now, thanks!

anpep requested review from benhoyt, paul-rodriguez, flotter and atesburak June 14, 2023 10:08

anpep self-assigned this Jun 14, 2023

atesburak reviewed Jun 14, 2023

View reviewed changes

benhoyt reviewed Jun 14, 2023

View reviewed changes

internals/cli/cmd_exec.go Outdated Show resolved Hide resolved

internals/cli/cmd_help.go Outdated Show resolved Hide resolved

internals/cli/cmd_help.go Outdated Show resolved Hide resolved

internals/cli/cmd_help.go Outdated Show resolved Hide resolved

anpep added 2 commits June 15, 2023 14:03

cli: remove unnecessary backticks from help

ec2da23

enter: include daemon name on panic()

2490d53

If the client panics due to the Pebble daemon stopping early, include the program name from personality so it's unambiguous.

anpep requested review from atesburak and benhoyt June 19, 2023 07:45

paul-rodriguez approved these changes Jun 19, 2023

View reviewed changes

benhoyt approved these changes Jun 19, 2023

View reviewed changes

flotter reviewed Jun 20, 2023

View reviewed changes

internals/cli/cmd_help.go Outdated Show resolved Hide resolved

flotter mentioned this pull request Jun 20, 2023

Preview of cli: make CLI name-agnostic flotter/pebble#3

Merged

benhoyt reviewed Jun 20, 2023

View reviewed changes

internals/cli/cli.go Show resolved Hide resolved

internals/cli/cli.go Outdated Show resolved Hide resolved

internals/cli/cmd_help.go Outdated Show resolved Hide resolved

internals/cli/cmd_run.go Outdated Show resolved Hide resolved

internals/cli/cmd_help.go Show resolved Hide resolved

help: minor changes and more robust manpage fix

1dcba69

paul-rodriguez approved these changes Jul 6, 2023

View reviewed changes

atesburak approved these changes Jul 11, 2023

View reviewed changes

cli: placeholder substitutions for personality

410136d

Argument, option and command descriptions can now utilize the `<display name>` and `<program name>` notation that will be substituted with the corresponding values depending on the current binary personality.

anpep requested a review from niemeyer July 24, 2023 08:03

paul-rodriguez approved these changes Jul 24, 2023

View reviewed changes

anpep mentioned this pull request Jul 31, 2023

client: public API #261

Closed

anpep added 2 commits August 7, 2023 11:13

Merge branch 'master' into feature/personality

4fae76b

cli: fix linting issues

c5628ff

paul-rodriguez approved these changes Aug 8, 2023

View reviewed changes

flotter approved these changes Aug 21, 2023

View reviewed changes

internals/cli/cmd_run.go Outdated Show resolved Hide resolved

run: Fix typo in command summary.

6daa931

anpep changed the title ~~cli: make CLI name-agnostic~~ feat(cli): implement personality for application name-agnostic help manuals Aug 23, 2023

paul-rodriguez approved these changes Aug 24, 2023

View reviewed changes

niemeyer requested changes Aug 25, 2023

View reviewed changes

anpep added 4 commits August 28, 2023 12:32

cli: Use cmdpkg convention for cmd import.

0284255

cli: Minor fixes.

f0ba2ed

Merge branch 'master' into feature/personality

1e8f2d9

paul-rodriguez approved these changes Sep 11, 2023

View reviewed changes

anpep requested a review from niemeyer September 11, 2023 13:03

anpep added the Reviewed label Nov 15, 2023

benhoyt requested changes Nov 15, 2023

View reviewed changes

anpep added 2 commits November 20, 2023 09:04

fix(cli): minor changes to personality

7bcbbc3

Merge branch 'master' into feature/personality

6c855d3

benhoyt requested changes Nov 20, 2023

View reviewed changes

anpep added 2 commits November 21, 2023 09:19

fix(cli): more personality changes

2371484

fix(cli): properly merge warnings code from master

b770a7e

paul-rodriguez reviewed Nov 21, 2023

View reviewed changes

internals/cli/cmd_warnings.go Outdated Show resolved Hide resolved

paul-rodriguez approved these changes Nov 21, 2023

View reviewed changes

benhoyt approved these changes Nov 21, 2023

View reviewed changes

benhoyt merged commit 4593177 into canonical:master Nov 21, 2023
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): implement personality for application name-agnostic help manuals #238

feat(cli): implement personality for application name-agnostic help manuals #238

anpep commented Jun 14, 2023 •

edited by benhoyt

Loading

atesburak Jun 14, 2023

benhoyt Nov 20, 2023

flotter left a comment

atesburak left a comment •

edited

Loading

anpep commented Jul 11, 2023 via email

anpep commented Jul 24, 2023

flotter left a comment

paul-rodriguez left a comment

niemeyer left a comment

anpep commented Nov 15, 2023

benhoyt left a comment

benhoyt Nov 15, 2023

anpep Nov 20, 2023

anpep Nov 20, 2023

benhoyt Nov 20, 2023

benhoyt Nov 15, 2023

benhoyt Nov 20, 2023

benhoyt Nov 15, 2023

anpep Nov 20, 2023

benhoyt Nov 20, 2023

anpep commented Nov 20, 2023

benhoyt commented Nov 20, 2023

benhoyt left a comment

benhoyt Nov 20, 2023

benhoyt Nov 20, 2023

benhoyt Nov 20, 2023

benhoyt Nov 20, 2023

benhoyt commented Nov 20, 2023

paul-rodriguez left a comment

benhoyt left a comment

feat(cli): implement personality for application name-agnostic help manuals #238

feat(cli): implement personality for application name-agnostic help manuals #238

Conversation

anpep commented Jun 14, 2023 • edited by benhoyt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flotter left a comment

Choose a reason for hiding this comment

atesburak left a comment • edited Loading

Choose a reason for hiding this comment

anpep commented Jul 11, 2023 via email

anpep commented Jul 24, 2023

flotter left a comment

Choose a reason for hiding this comment

paul-rodriguez left a comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

anpep commented Nov 15, 2023

benhoyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anpep commented Nov 20, 2023

benhoyt commented Nov 20, 2023

benhoyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhoyt commented Nov 20, 2023

paul-rodriguez left a comment

Choose a reason for hiding this comment

benhoyt left a comment

Choose a reason for hiding this comment

anpep commented Jun 14, 2023 •

edited by benhoyt

Loading

atesburak left a comment •

edited

Loading