-
Notifications
You must be signed in to change notification settings - Fork 61
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
Conversation
Like @woky mentioned in #188,
|
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
f799c39
to
906b6df
Compare
There was a problem hiding this 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:
- 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. - 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.
There was a problem hiding this 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.
There was a problem hiding this 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.
force-pushed new commit: rebornplusplus/go-flags@18b729e
There was a problem hiding this 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:
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. LGTM.
canonical/go-flags#2 has been merged. Point to the recent commit canonical/go-flags@01157a7.
There was a problem hiding this 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.
.. include the new changes in usptream into this PR.
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) |
There was a problem hiding this 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!
There was a problem hiding this 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
There was a problem hiding this 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``.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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.
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:
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 thefind
command-line utility,we propose adding a new option to
pebble run
, called--args
.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”:
Start the Pebble server with all services on hold, and give some
args to “svc”:
Start the Pebble server and pass arguments to multiple services: