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

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

Merged
merged 16 commits into from
Nov 21, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions cmd/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,9 @@ func MockVersion(version string) (restore func()) {
Version = version
return func() { Version = old }
}

// ProgramName represents the name of the application binary.
var ProgramName string = "pebble"

// DisplayName represents the user-facing name of the application.
var DisplayName string = "Pebble"
28 changes: 17 additions & 11 deletions internals/cli/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"golang.org/x/crypto/ssh/terminal"

"github.com/canonical/pebble/client"
"github.com/canonical/pebble/cmd"
anpep marked this conversation as resolved.
Show resolved Hide resolved
"github.com/canonical/pebble/internals/logger"
)

Expand Down Expand Up @@ -165,8 +166,8 @@ func Parser(cli *client.Client) *flags.Parser {

flagOpts := flags.Options(flags.PassDoubleDash)
parser := flags.NewParser(&defaultOpts, flagOpts)
parser.ShortDescription = "Tool to interact with pebble"
parser.LongDescription = longPebbleDescription
parser.ShortDescription = "System and service manager"
parser.LongDescription = applyPersonality(longPebbleDescription)

// Add --help like what go-flags would do for us, but hidden
addHelp(parser)
Expand All @@ -192,7 +193,7 @@ func Parser(cli *client.Client) *flags.Parser {
} else {
target = parser.Command
}
cmd, err := target.AddCommand(c.Name, c.Summary, strings.TrimSpace(c.Description), obj)
cmd, err := target.AddCommand(c.Name, applyPersonality(c.Summary), applyPersonality(strings.TrimSpace(c.Description)), obj)
if err != nil {
logger.Panicf("internal error: cannot add command %q: %v", c.Name, err)
}
Expand All @@ -203,9 +204,9 @@ func Parser(cli *client.Client) *flags.Parser {
positionalHelp := map[string]string{}
for specifier, help := range c.ArgsHelp {
if flagRegexp.MatchString(specifier) {
flagHelp[specifier] = help
flagHelp[specifier] = applyPersonality(help)
} else if positionalRegexp.MatchString(specifier) {
positionalHelp[specifier] = help
positionalHelp[specifier] = applyPersonality(help)
} else {
logger.Panicf("internal error: invalid help specifier from %q: %q", c.Name, specifier)
}
Expand All @@ -219,10 +220,10 @@ func Parser(cli *client.Client) *flags.Parser {
for _, opt := range opts {
if description, ok := flagHelp["--"+opt.LongName]; ok {
lintDesc(c.Name, opt.LongName, description, opt.Description)
opt.Description = description
opt.Description = applyPersonality(description)
} else if description, ok := flagHelp["-"+string(opt.ShortName)]; ok {
lintDesc(c.Name, string(opt.ShortName), description, opt.Description)
opt.Description = description
opt.Description = applyPersonality(description)
} else if !opt.Hidden {
logger.Panicf("internal error: %q missing description for %q", c.Name, opt)
}
Expand All @@ -240,6 +241,11 @@ func Parser(cli *client.Client) *flags.Parser {
return parser
}

func applyPersonality(s string) string {
r := strings.NewReplacer("{{.ProgramName}}", cmd.ProgramName, "{{.DisplayName}}", cmd.DisplayName)
benhoyt marked this conversation as resolved.
Show resolved Hide resolved
return r.Replace(s)
}

var (
isStdinTTY = terminal.IsTerminal(0)
isStdoutTTY = terminal.IsTerminal(1)
Expand Down Expand Up @@ -271,7 +277,7 @@ func Run() error {
}
}()

logger.SetLogger(logger.New(os.Stderr, "[pebble] "))
logger.SetLogger(logger.New(os.Stderr, fmt.Sprintf("[%s] ", cmd.ProgramName)))

_, clientConfig.Socket = getEnvPaths()

Expand All @@ -293,11 +299,11 @@ func Run() error {
return nil
case flags.ErrUnknownCommand:
sub := os.Args[1]
sug := "pebble help"
sug := cmd.ProgramName + " help"
if len(xtra) > 0 {
sub = xtra[0]
if x := parser.Command.Active; x != nil && x.Name != "help" {
sug = "pebble help " + x.Name
sug = cmd.ProgramName + " help " + x.Name
}
}
return fmt.Errorf("unknown command %q, see '%s'", sub, sug)
Expand Down Expand Up @@ -341,7 +347,7 @@ func errorToMessage(e error) (normalMessage string, err error) {
}
case client.ErrorKindSystemRestart:
isError = false
msg = "pebble is about to reboot the system"
msg = fmt.Sprintf("%s is about to reboot the system", cmd.DisplayName)
case client.ErrorKindNoDefaultServices:
msg = "no default services"
default:
Expand Down
4 changes: 2 additions & 2 deletions internals/cli/cmd_enter.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (

const cmdEnterSummary = "Run subcommand under a container environment"
const cmdEnterDescription = `
The enter command facilitates the use of Pebble as an entrypoint for containers.
The enter command facilitates the use of {{.DisplayName}} as an entrypoint for containers.
When used without a subcommand it mimics the behavior of the run command
alone, while if used with a subcommand it runs that subcommand in the most
appropriate environment taking into account its purpose.
Expand Down Expand Up @@ -178,7 +178,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")

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
panic(runPanic)
}
Expand Down
2 changes: 1 addition & 1 deletion internals/cli/cmd_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ stderr are output locally.
To avoid confusion, exec options may be separated from the command and its
arguments using "--", for example:

pebble exec --timeout 10s -- echo -n foo bar
{{.ProgramName}} exec --timeout 10s -- echo -n foo bar
`

type cmdExec struct {
Expand Down
38 changes: 21 additions & 17 deletions internals/cli/cmd_help.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import (
"unicode/utf8"

"github.com/canonical/go-flags"

cmdpkg "github.com/canonical/pebble/cmd"
)

const cmdHelpSummary = "Show help about a command"
Expand Down Expand Up @@ -103,17 +105,19 @@ func (cmd *cmdHelp) setParser(parser *flags.Parser) {
// - duplicated TP lines that break older groff (e.g. 14.04), lp:1814767
type manfixer struct {
bytes.Buffer
done bool
done bool
programName string
}

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

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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

if bytes.HasPrefix(buf, []byte(prefix)) {
// io.Writer.Write must not modify the buffer, even temporarily
anpep marked this conversation as resolved.
Show resolved Hide resolved
n, _ := w.Buffer.Write(buf[:9])
n, _ := w.Buffer.Write(buf[:len(prefix)])
w.Buffer.Write([]byte{'8'})
m, err := w.Buffer.Write(buf[10:])
m, err := w.Buffer.Write(buf[1+len(prefix):])
return n + m + 1, err
}
}
Expand All @@ -134,7 +138,7 @@ func (cmd cmdHelp) Execute(args []string) error {
if cmd.Manpage {
// you shouldn't try to to combine --man with --all nor a
// subcommand, but --man is hidden so no real need to check.
out := &manfixer{}
out := &manfixer{programName: cmd.parser.Name}
cmd.parser.WriteManPage(out)
out.flush()
return nil
Expand All @@ -151,9 +155,9 @@ func (cmd cmdHelp) Execute(args []string) error {
for _, subname := range cmd.Positional.Subs {
subcmd = subcmd.Find(subname)
if subcmd == nil {
sug := "pebble help"
sug := cmdpkg.ProgramName + " help"
if x := cmd.parser.Command.Active; x != nil && x.Name != "help" {
sug = "pebble help " + x.Name
sug = cmdpkg.ProgramName + " help " + x.Name
}
return fmt.Errorf("unknown command %q, see '%s'.", subname, sug)
}
Expand All @@ -175,7 +179,7 @@ type HelpCategory struct {
// HelpCategories helps us by grouping commands
var HelpCategories = []HelpCategory{{
Label: "Run",
Description: "run pebble",
Description: "run the service manager",
Commands: []string{"run", "help", "version"},
}, {
Label: "Plan",
Expand Down Expand Up @@ -205,35 +209,35 @@ var HelpCategories = []HelpCategory{{

var (
longPebbleDescription = strings.TrimSpace(`
Pebble lets you control services and perform management actions on
{{.DisplayName}} lets you control services and perform management actions on
the system that is running them.
`)
pebbleUsage = "Usage: pebble <command> [<options>...]"
pebbleUsage = "Usage: {{.ProgramName}} <command> [<options>...]"
pebbleHelpCategoriesIntro = "Commands can be classified as follows:"
pebbleHelpAllFooter = "Set the PEBBLE environment variable to override the configuration directory \n" +
"(which defaults to " + defaultPebbleDir + "). Set PEBBLE_SOCKET to override \n" +
"the unix socket used for the API (defaults to $PEBBLE/.pebble.socket).\n" +
"\n" +
"For more information about a command, run 'pebble help <command>'."
pebbleHelpFooter = "For a short summary of all commands, run 'pebble help --all'."
"For more information about a command, run '{{.ProgramName}} help <command>'."
pebbleHelpFooter = "For a short summary of all commands, run '{{.ProgramName}} help --all'."
)

func printHelpHeader() {
fmt.Fprintln(Stdout, longPebbleDescription)
fmt.Fprintln(Stdout, applyPersonality(longPebbleDescription))
fmt.Fprintln(Stdout)
fmt.Fprintln(Stdout, pebbleUsage)
fmt.Fprintln(Stdout, applyPersonality(pebbleUsage))
fmt.Fprintln(Stdout)
fmt.Fprintln(Stdout, pebbleHelpCategoriesIntro)
fmt.Fprintln(Stdout, applyPersonality(pebbleHelpCategoriesIntro))
}

func printHelpAllFooter() {
fmt.Fprintln(Stdout)
anpep marked this conversation as resolved.
Show resolved Hide resolved
fmt.Fprintln(Stdout, pebbleHelpAllFooter)
fmt.Fprintln(Stdout, applyPersonality(pebbleHelpAllFooter))
}

func printHelpFooter() {
printHelpAllFooter()
fmt.Fprintln(Stdout, pebbleHelpFooter)
fmt.Fprintln(Stdout, applyPersonality(pebbleHelpFooter))
}

// this is called when the Execute returns a flags.Error with ErrCommandRequired
Expand Down
2 changes: 1 addition & 1 deletion internals/cli/cmd_help_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func (s *PebbleSuite) TestHelpMan(c *C) {

err := cli.RunMain()
c.Assert(err, Equals, nil)
c.Check(s.Stdout(), Matches, `(?s)\.TH.*\.SH NAME.*pebble \\- Tool to interact with pebble.*`)
c.Check(s.Stdout(), Matches, `(?s)\.TH pebble 8.*\.SH NAME.*pebble \\- System and service manager.*`)
c.Check(s.Stderr(), Equals, "")
}

Expand Down
4 changes: 2 additions & 2 deletions internals/cli/cmd_okay.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ import (
const cmdOkaySummary = "Acknowledge notices and warnings"
const cmdOkayDescription = `
The okay command acknowledges warnings and notices that have been previously
listed using 'pebble warnings' or 'pebble notices', so that they are omitted
listed using '{{.ProgramName}} warnings' or '{{.ProgramName}} notices', so that they are omitted
from future runs of either command. When a notice or warning is repeated, it
will again show up until the next 'pebble okay'.
will again show up until the next '{{.ProgramName}} okay'.
`

type cmdOkay struct {
Expand Down
2 changes: 1 addition & 1 deletion internals/cli/cmd_plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (

var cmdPlanSummary = "Show the plan with layers combined"
var cmdPlanDescription = `
The plan command prints out the effective configuration of pebble in YAML
The plan command prints out the effective configuration of {{.DisplayName}} in YAML
format. Layers are combined according to the override rules defined in them.
`

Expand Down
10 changes: 5 additions & 5 deletions internals/cli/cmd_run.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,16 @@ import (
"github.com/canonical/pebble/internals/systemd"
)

const cmdRunSummary = "Run the pebble environment"
const cmdRunSummary = "Run the service manager environment"
const cmdRunDescription = `
The run command starts pebble and runs the configured environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


Additional arguments may be provided to the service command with the --args option, which
must be terminated with ";" unless there are no further Pebble options. These arguments
must be terminated with ";" unless there are no further program options. These arguments
are appended to the end of the service command, and replace any default arguments defined
in the service plan. For example:

$ pebble run --args myservice --port 8080 \; --hold
$ {{.ProgramName}} run --args myservice --port 8080 \; --hold
`

type sharedRunEnterOpts struct {
Expand All @@ -52,7 +52,7 @@ type sharedRunEnterOpts struct {
}

var sharedRunEnterArgsHelp = map[string]string{
"--create-dirs": "Create pebble directory on startup if it doesn't exist",
"--create-dirs": "Create {{.DisplayName}} directory on startup if it doesn't exist",
"--hold": "Do not start default services automatically",
"--http": `Start HTTP API listening on this address (e.g., ":4000")`,
"--verbose": "Log all output from services to stdout",
Expand Down Expand Up @@ -98,7 +98,7 @@ func (rcmd *cmdRun) run(ready chan<- func()) {
// This exit code must be in system'd SuccessExitStatus.
panic(&exitStatus{42})
}
fmt.Fprintf(os.Stderr, "cannot run pebble: %v\n", err)
fmt.Fprintf(os.Stderr, "cannot run daemon: %v\n", err)
panic(&exitStatus{1})
}
}
Expand Down
2 changes: 1 addition & 1 deletion internals/cli/cmd_signal.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this should be indented?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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-

`

type cmdSignal struct {
Expand Down
11 changes: 6 additions & 5 deletions internals/cli/cmd_warnings.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,15 @@ import (
"github.com/canonical/x-go/strutil/quantity"

"github.com/canonical/pebble/client"
"github.com/canonical/pebble/cmd"
"github.com/canonical/pebble/internals/osutil"
)

const cmdWarningsSummary = "List warnings"
const cmdWarningsDescription = `
The warnings command lists the warnings that have been reported to the system.

Once warnings have been listed with 'pebble warnings', 'pebble okay' may be used to
Once warnings have been listed with '{{.ProgramName}} warnings', '{{.ProgramName}} okay' may be used to
silence them. A warning that's been silenced in this way will not be listed
again unless it happens again, _and_ a cooldown time has passed.

Expand Down Expand Up @@ -196,7 +197,7 @@ func lastWarningTimestamp() (time.Time, error) {
f, err := os.Open(warnFilename(user.HomeDir))
if err != nil {
if os.IsNotExist(err) {
return time.Time{}, nil
return time.Time{}, fmt.Errorf("you must have looked at the warnings before acknowledging them. Try 'pebble warnings'.")
anpep marked this conversation as resolved.
Show resolved Hide resolved
}
return time.Time{}, fmt.Errorf("cannot open timestamp file: %v", err)
}
Expand All @@ -220,9 +221,9 @@ func maybePresentWarnings(count int, timestamp time.Time) {
return
}

format := "WARNING: There are %d new warnings. See 'pebble warnings'.\n"
format := "WARNING: There are %d new warnings. See '%s warnings'.\n"
if count == 1 {
format = "WARNING: There is %d new warning. See 'pebble warnings'.\n"
format = "WARNING: There is %d new warning. See '%s warnings'.\n"
}
fmt.Fprintf(Stderr, format, count)
fmt.Fprintf(Stderr, format, count, cmd.ProgramName)
}
Loading
Loading