-
Notifications
You must be signed in to change notification settings - Fork 11
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(debian): Add example broker binary #224
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #224 +/- ##
==========================================
- Coverage 86.23% 86.19% -0.04%
==========================================
Files 65 65
Lines 5106 5106
Branches 9 9
==========================================
- Hits 4403 4401 -2
- Misses 489 490 +1
- Partials 214 215 +1 ☔ View full report in Codecov by Sentry. |
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.
Before we go on on that one:
- Code sanity is failing
- From our previous discussion in the previous PR, we were ok to land this new binary BUT not to have a systemd unit, even masks.
The goal was to have the example runner, when being started *manually, to drop its config, and clean it up on exit.
That way, we really signifies this is a manual/automated test tool, not to be used by anything else. Then, the test setup can drop a temporary systemd unit if we need to test reboot.
AFAIK, this is not planned work. Where is the task in Jira for this work? |
61750a0
to
4dc6bea
Compare
It was just a temporary issue, it's green now.
Well, I didn't get a no on my proposal on relying on systemd to handle that.
However things can be dropped, but IMHO not doing that implies that we can't test the ability of authd to dbus-activate the example daemon when the user selects it, that I feel is something would be nice to be able to trigger. And having to do But as you said, that can indeed be done at test phase too. So I guess I'll just install the systemd service in debian |
No, it wasn't but it was the natural outcome of having a PPA, since I had to do 90% of this work to get a testing PPA working with a testable broker, thus I decided to split that work into another PR that is something still need to have proper integration tests in a autopkgtests. |
yes, and same for the configuration. I still think the binary itself can install/remove the configuration if it’s not there are startup time for easy testing. |
I'm marking this as draft and rebase on #223 so that review should be easier once that one lands... |
03bfcad
to
713c5fa
Compare
713c5fa
to
60c051b
Compare
60c051b
to
9f6f9a9
Compare
#!/usr/bin/dh-exec | ||
|
||
usr/bin/examplebroker-bin => ${env:AUTHD_DAEMONS_PATH}/authd-examplebroker | ||
|
||
examplebroker/com.ubuntu.authd.ExampleBroker.conf /usr/share/dbus-1/system.d | ||
examplebroker/com.ubuntu.authd.ExampleBroker.service /usr/share/dbus-1/system-services |
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.
I think this is still not what we want. IMHO, the examplebroker should be an executable that, when executed, exports the broker on dbus
and cleans it up when the program finishes running. This way, we don't need to worry about having to install/uninstall it. Its purpose is to test authd
, not the broker behaviour, so we don't need anything directly configured on dbus
(as authd
does not directly care about it).
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.
I see the point, but that wouldn't allow proper testing, especially if we want to do proper autopkgtests were you just install everything and then you run a client that would work as the system would be configured properly.
so we don't need anything directly configured on dbus (as authd does not directly care about it).
In fact authd
does care about it, because if that would happen when we run the daemon, then we wouldn't be able of testing the case that authd does dbus-activation of the service when that you select the broker on the brokers list.
Also, not having a script like this would imply that we rely on the autopkgtest script to do that, but IMHO it's not nice to maintain the same in two different places.
What we may want to do, in case, is to make the examplebrorker to do the conf setup in case that's not set, and I can do that, but I feel we should still provide a way to create what would be the real setup in a properly configured machine.
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.
Ah, also having those files installed doesn't allow anything but just having the service to own the name, and while these may be handled by the service itself, as I said, I'd prefer to be able to do dbus-activation (but only through systemd that it's not installed by default).
In order to do integration tests, autopkgtests or manual tests we may need to have a simple broker installed. So expose the example broker as a separate binary pacakge so that we can easily install it when required.
Using default names would make those files to use the first package, but let's be cleaner.
These are now installed as examples and can be manually added to the proper locations for testing purposes
This wasn't supported by older dh-exec but it is now, so let's use it to be more consistent with other files
9f6f9a9
to
10b16e8
Compare
This adds a new binary package with the example broker, but with these limitations:
The broker is masked on systemd by default so even installing the package, one need to usesystemclt unmask authd-example-broker
to get it working/usr/share/doc/authd-example-broker/examples/authd-example-broker.installer.sh install
UDENG-2382