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: Provide arguments to service via pebble run --args #191

Merged
merged 23 commits into from
May 22, 2023

Conversation

rebornplusplus
Copy link
Member

The goal is to introduce the concept of optional and overrideable
arguments for a Pebble service, but we must ensure that the layering
properties of Pebble are preserved and respected. The proposed syntax
re-uses therefore the existing service “command” attribute:

services:
    myservice:
        command: myservice --db=/var/db [ --quiet ]

The CLI is extended to support replacing the default command
arguments defined in the plan, with the ones given at execution time.
Inspired by the -exec syntax of the find command-line utility,
we propose adding a new option to pebble run, called --args.

Usage:
    pebble run --args SERVICE [ARGS ...] [TERMINATOR]

SERVICE:      required name of the Pebble service that will
              receive the arguments;
[ARGS ...]:   arguments to be passed to the Pebble service named
              “SERVICE”;
[TERMINATOR]: a “;” symbol (backslash-escaped if using the 
              shell) to indicate that whatever follows is not 
              an argument for “SERVICE”
                  - this symbol is only required whenever other 
                    Pebble-specific options and/or commands are 
                    given after --args, otherwise can be omitted.

With this, we can provide additional arguments to multiple services
defined in the plan, whether they hold default arguments or not.

Examples:

Start the Pebble server and only pass some arguments to “svc”:

pebble run --args svc --verbose --foo "multi str arg"

Start the Pebble server with all services on hold, and give some
args to “svc”:

pebble run --args svc --verbose --foo "multi str arg" \; --hold

Start the Pebble server and pass arguments to multiple services:

pebble run --args svc1 --verbose --foo "multi str arg" \; \
           --args svc2 --somearg 1 --payload '{"foo": "bar"}'

@rebornplusplus
Copy link
Member Author

rebornplusplus commented Feb 8, 2023

Like @woky mentioned in #188,
I have included https://github.com/rebornplusplus/go-flags
which supports terminated options like --args here. There is a PR
against upstream jessevdk/go-flags#395. But
I am not sure when it will get any response from the maintainer.
Should we consider forking go-flags and use that for now?

The conflicts in this PR come due to the usage of fork. (rebased)
Also, I have a few scattered commits here. I intend to squash
them before they are okay to go in.

The goal is to introduce the concept of optional and
overrideable arguments for a Pebble service, but we
must ensure that the layering properties of Pebble
are preserved and respected. The proposed syntax re-uses
therefore the existing service “command” attribute:

    services:
	myservice:
	    command: myservice --db=/var/db [ --quiet ]

The markers [ and ] are suggested because these are
typical annotations for optional parameters and are
rarely seen naked because they are interpreted by shells.
Arguments to a service can now be passed by
using the following syntax:

    pebble run [opts..] --args <svc> arguments.. ; [opts..]

Tests are failing right now, will be fixed soon.
The upstream go-flags project do not support terminated options
like ``find -exec``. The repo was forked and the feature was
introduced in [0].

Until the PR [1] is merged in the upstream repo, we need to
maintain and point to a fork with the features there.

Refs:
- [0] rebornplusplus/go-flags@1dbaf44
- [1] jessevdk/go-flags#395
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

This looks like a good first pass, thanks! I've left a bunch of comments/suggestions, with two main structural concerns:

  1. I think we can simplify significantly just by avoiding the API/client stuff, and pass --args to the daemon (and from there, to the service manager) directly.
  2. I think we should separate out the CmdArgs handling from the plan, as it's not really related to the plan, but a separate override of the plan. I think it'll clean it up and probably simplify to extract this.

@rebornplusplus
Copy link
Member Author

Hi @benhoyt, I tried to address your comments (they were super helpful!) in de12f37. Please take a look and let me know what you think. Thank you!

Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Looking much better, thanks! I'm approving but with a few minor suggestions for some text clarity and a simplification about how you're passing the args down to overlord and service manager.

Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Excellent, thanks for that -- looks good to me now.

Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Thanks for this Rafid. It's looking very promising.

Here is a first pass:

@rebornplusplus
Copy link
Member Author

Hi @niemeyer, thanks for your insightful comments. I have added a commit addressing them. Will you please take another look?

Additionally, the new commit will require the PR canonical/x-go#19 to go in. Will you please take a look there as well? That one is awaiting one more approval.

@rebornplusplus rebornplusplus requested a review from niemeyer March 14, 2023 07:19
@rebornplusplus
Copy link
Member Author

rebornplusplus commented Mar 14, 2023

Also, I have added a PR in canonical/go-flags#2 which will be needed for this PR as well. It will be really helpful if you can check it too.

Copy link
Contributor

@woky woky left a comment

Choose a reason for hiding this comment

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

Nice. LGTM.

Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, Rafid, this is much closer.

Here is a second pass:

.. and a few rewording and cleanup.
@cjdcordeiro cjdcordeiro added the High Priority Look at me first label Apr 11, 2023
.. include the new changes in usptream into this PR.
@rebornplusplus
Copy link
Member Author

Note about the failing "Tests / Root Tests": It passes in my GitHub repo and local workstation. https://github.com/rebornplusplus/pebble/actions/runs/4675467143 (successful GH action for the same set of commits)

Copy link
Contributor

@woky woky left a comment

Choose a reason for hiding this comment

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

@niemeyer @rebornplusplus @cjdcordeiro The test failure was not fault of this PR. Now all checks are passing.

LGTM!

@cjdcordeiro cjdcordeiro removed their request for review April 20, 2023 08:59
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Thanks! Two small requests: 1) label, and 2) test SetServiceArgs

@rebornplusplus rebornplusplus requested a review from benhoyt May 3, 2023 10:26
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Looking good now, thank you.

Probably the last pass:

Update error messages in ``plan.Service.ParseCommand`` for
more context. Additionally, update the TODO comment in
``servstate.ServiceManager.SetServiceArgs``.
Note that tests will fail, since the new commit in
canonical/x-go is not included **yet**.
As canonical/x-go#19 is merged,
update the canonical/x-go module version to the latest commit.

Also run ``go mod tidy``.
@rebornplusplus
Copy link
Member Author

Hiya @niemeyer. Thanks for the reviews. I have updated the requested changes in the recent commits. I have updated the version of the canonical/x-go module as well. Tests are passing.
There have been a bunch of commits in this PR and when the PR is ready to be merged, I would like to squash the commits into one before merging.

Copy link
Contributor

@woky woky left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Thank you! Great to see this going in.

@niemeyer niemeyer merged commit 8aa9c04 into canonical:master May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High Priority Look at me first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants