Add the option to add flags to custom commands, fixes #2491

[gilbertsoft]

The Problem/Issue/Bug:

More and more bundled custom commands are providing flags but there is not option to describe this flags in the custom command using the cobra definition which prints the flags for a command.

How this PR Solves The Problem:

A new option Flags is recognized now to describe the flags with a JSON comment in the custom command.

Example:

## Flags: [{"Name":"test","Shorthand":"t","Usage":"Usage of test"}]

The Shorthand is optional and can be omitted.

Manual Testing Instructions:

Create a new custom command and add a flags definition to the comment header see above or the detailed explanation in the docs.

Automated Testing Overview:

The provided tests fully cover all cases including errors and panics.

Related Issue Link(s):

Closes #2491

Release/Deployment notes:

Nothing special is needed.

[rfay]

The failure in CircleCI is in https://app.circleci.com/pipelines/github/drud/ddev/2744/workflows/36fa626d-ad99-4c90-a739-670728b3ab85/jobs/27496 -

You can run make staticrequired to verify gofmt issues, and I try to link up the pre-push hook, see https://github.com/drud/ddev/blob/master/.githooks/README.md - to avoid this happening. It's a little annoying waiting for the push, but OTOH better than finding out later that the build failed. I do the ln -s ../../.githooks/pre-push.allchecks pre-push

[gilbertsoft]

The failure in CircleCI is in https://app.circleci.com/pipelines/github/drud/ddev/2744/workflows/36fa626d-ad99-4c90-a739-670728b3ab85/jobs/27496 -

You can run make staticrequired to verify gofmt issues, and I try to link up the pre-push hook, see https://github.com/drud/ddev/blob/master/.githooks/README.md - to avoid this happening. It's a little annoying waiting for the push, but OTOH better than finding out later that the build failed. I do the ln -s ../../.githooks/pre-push.allchecks pre-push

Yeah thanks! I've added more go extensions to VS Code which should also solve this issue :)

[gilbertsoft]

@rfay Change is ready to review now. Will add the docs once #2494 and #2495 are merged....

[gilbertsoft]

@rfay examples to test the flags can be found in the tests as starting point

[rfay]

Minor change requests.

Have you looked at the performance impact at all? ddev is already doing way too much with these commands every time ddev is executed, which is already worrisome.

I do see a persistent failure, didn't debug it, it's the fail that's breaking the tests:

Error 'invalid character '[' looking for beginning of object key string', command 'test' contains an invalid flags definition '{[{"Name":"test","Shorthand":"t","Usage":"Usage of test"}]', skipping add flags of /Users/rfay/workspace/d9composer/.ddev/.global_commands/web/test

[gilbertsoft]

Have you looked at the performance impact at all? ddev is already doing way too much with these commands every time ddev is executed, which is already worrisome.

No, I did not. But saw this behavior during the development which is really not so nice. I have an idea how we can improve this situation but I first have to check if this works at all. Should be done in a next or prior change imho.

I do see a persistent failure, didn't debug it, it's the fail that's breaking the tests:

Error 'invalid character '[' looking for beginning of object key string', command 'test' contains an invalid flags definition '{[{"Name":"test","Shorthand":"t","Usage":"Usage of test"}]', skipping add flags of /Users/rfay/workspace/d9composer/.ddev/.global_commands/web/test

Should be fixed now.

[rfay]

I created this global web command named "test"

#!/bin/bash

## Description: test
## Usage: test
## Flags: {[{"Name":"test","Shorthand":"t","Usage":"Usage of test"}]
## Example: "ddev test"

echo "test was executed with args=$@ on host $(hostname)"

But ddev help test doesn't tell me anything about flags:

$ ddev test --help
test (global shell web container command)

Usage:
  ddev test [flags]

Examples:
  "ddev test"

Flags:
  -h, --help   help for test

Global Flags:
  -j, --json-output   If true, user-oriented output will be in JSON format.

[gilbertsoft]

I did the same like you and get this expectet output:

PS E:\drud\ddev\.gotmp\bin\windows_amd64> .\ddev test -h
Error 'invalid character '[' looking for beginning of object key string', command 'test' contains an invalid flags definition '{[{"Name":"test","Shorthand":"t","Usage":"Usage of test"}]', skipping add flags of E:\drud\ddev\.gotmp\bin\windows_amd64\.ddev\commands\web\test
test (shell web container command)

Usage:
  ddev test [flags]

Examples:
  "ddev test"

Flags:
  -h, --help   help for test

Global Flags:
  -j, --json-output   If true, user-oriented output will be in JSON format.

There is a error in the JSON, the starting { should be removed. Then I get the following:

PS E:\drud\ddev\.gotmp\bin\windows_amd64> .\ddev test -h
test (shell web container command)

Usage:
  ddev test [flags]

Examples:
  "ddev test"

Flags:
  -h, --help   help for test
  -t, --test   Usage of test

Global Flags:
  -j, --json-output   If true, user-oriented output will be in JSON format.

Did you really not get an error message like me? That would mean you somehow use the wrong binary... BTW I've tested with project and global commands, same result.

[rfay]

OK. I took that from https://github.com/gilbertsoft/ddev/blob/21082e1b30b802728df129530884dd182b16f600/cmd/ddev/cmd/testdata/TestCustomCommands/project_commands/host/testhostcmdflags#L5, and I must have messed it up somehow.

I'm not entirely sure that people will be comfortable with json here but I'm OK to go with it.

I'll take another look when the tests pass.

[gilbertsoft]

OK. I took that from https://github.com/gilbertsoft/ddev/blob/21082e1b30b802728df129530884dd182b16f600/cmd/ddev/cmd/testdata/TestCustomCommands/project_commands/host/testhostcmdflags#L5, and I must have messed it up somehow.

Yeah, this one was buggy and made the tests failing :(

I'm not entirely sure that people will be comfortable with json here but I'm OK to go with it.

I'm open for other ideas here but it seems to be the easiest. And nowadays people should be able to write such simple JSON. To support less experienced people I added the whole validation thing which outputs as clear as possible error messages for user misstakes... That's why I'm wondering you did not see / post the error message which should have pointed you to the wrong JSON definition yesterday.

[gilbertsoft]

Two good and working examples can be found here:

• https://github.com/drud/ddev/pull/2493/files#diff-8d9431d2887832d2de5cfd81bbfe6495R48
• https://github.com/drud/ddev/pull/2493/files#diff-8d9431d2887832d2de5cfd81bbfe6495R58

[rfay]

Those links aren't helpful, sorry. Must be a copy/paste problem? Please link to code in your fork rather than to a files diff in PR.

Here's what https://github.com/drud/ddev/pull/2493/files#diff-8d9431d2887832d2de5cfd81bbfe6495R48 results in:

[gilbertsoft]

Those links aren't helpful, sorry. Must be a copy/paste problem? Please link to code in your fork rather than to a files diff in PR.

Links are fine but the browser jumps here to the wrong line. Here the two examples I'd like to show you:

• [{"Name":"test","Usage":"Usage of test"}]
• [{"Name":"test-1","Shorthand":"t","Usage":"Usage of test 1","Type":"string","DefValue":"true","NoOptDefVal":"true","Annotations":{"test-1":["test-1-1","test-1-2"]}},{"Name":"test-2","Usage":"Usage of test 2","Type":"bool","DefValue":"true","NoOptDefVal":"true","Annotations":{"test-2":["test-2-1","test-2-2"]}}]

The annotation in the second one is not useful, it was meant for testing only. But the enhanced docs have a link to spf13 where this is explained if you like to test it. It's for autocompletion only so not so imortant in the first place but I thought if somebody likes to use it e.g. to define possible file extensions for a file name paramter...

[rfay]

This should be a bit stronger. I know you're planning another PR or two, so let's not delay this one, but when you next work on commands, let's make this say something like "THIS DOES NOT DEFINE THE FLAG HANDLING, ONLY WHAT COMES OUT IN THE HELP"

[gilbertsoft]

Yes, agree...

[rfay]

Looking good! Thanks for all the great work on this.