Skip to content

feat(debian): Add example broker binary#224

Closed
3v1n0 wants to merge 12 commits intocanonical:mainfrom
3v1n0:example-broker-binary
Closed

feat(debian): Add example broker binary#224
3v1n0 wants to merge 12 commits intocanonical:mainfrom
3v1n0:example-broker-binary

Conversation

@3v1n0
Copy link
Copy Markdown
Contributor

@3v1n0 3v1n0 commented Feb 23, 2024

This adds a new binary package with the example broker, but with these limitations:

  • The service is not enabled nor started ever during installing phase
  • The broker is masked on systemd by default so even installing the package, one need to use systemclt unmask authd-example-broker to get it working
  • The broker systemd service is not installed by default, so to be enabled you need to manually run /usr/share/doc/authd-example-broker/examples/authd-example-broker.installer.sh install

UDENG-2382

@3v1n0 3v1n0 requested a review from a team as a code owner February 23, 2024 17:07
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.19%. Comparing base (ba8bb9f) to head (10b16e8).
Report is 3 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown
Contributor

@didrocks didrocks left a 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.

@jibel
Copy link
Copy Markdown
Contributor

jibel commented Feb 26, 2024

AFAIK, this is not planned work. Where is the task in Jira for this work?
Besides what is the rationale behind this change?

@3v1n0 3v1n0 force-pushed the example-broker-binary branch from 61750a0 to 4dc6bea Compare February 26, 2024 12:08
@3v1n0
Copy link
Copy Markdown
Contributor Author

3v1n0 commented Feb 26, 2024

Before we go on on that one:

* Code sanity is failing

It was just a temporary issue, it's green now.

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

Well, I didn't get a no on my proposal on relying on systemd to handle that.

The goal was to have the example runner, when being started *manually, to drop its config, and clean it up on exit.

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 systemctl unmaks before of any of that (even after having installed the package) looks to me a quite strong protection.

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 examples for an easy copy&paste, is that fine?

@3v1n0
Copy link
Copy Markdown
Contributor Author

3v1n0 commented Feb 26, 2024

AFAIK, this is not planned work. Where is the task in Jira for this work? Besides what is the rationale behind this change?

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.

@didrocks
Copy link
Copy Markdown
Contributor

didrocks commented Feb 26, 2024

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 examples for an easy copy&paste, is that fine?

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.

@3v1n0
Copy link
Copy Markdown
Contributor Author

3v1n0 commented Feb 26, 2024

I'm marking this as draft and rebase on #223 so that review should be easier once that one lands...

@3v1n0 3v1n0 marked this pull request as draft February 26, 2024 16:50
@3v1n0 3v1n0 force-pushed the example-broker-binary branch 2 times, most recently from 03bfcad to 713c5fa Compare February 26, 2024 21:15
@3v1n0 3v1n0 force-pushed the example-broker-binary branch from 713c5fa to 60c051b Compare March 5, 2024 11:15
@3v1n0 3v1n0 marked this pull request as ready for review March 5, 2024 11:17
@3v1n0 3v1n0 force-pushed the example-broker-binary branch from 60c051b to 9f6f9a9 Compare March 5, 2024 11:17
@3v1n0 3v1n0 requested a review from didrocks March 5, 2024 11:20
Comment on lines +1 to +6
#!/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
Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

@3v1n0 3v1n0 Mar 8, 2024

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.

Copy link
Copy Markdown
Contributor Author

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

3v1n0 added 12 commits March 27, 2024 08:13
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
@3v1n0 3v1n0 force-pushed the example-broker-binary branch from 9f6f9a9 to 10b16e8 Compare March 27, 2024 07:15
@3v1n0
Copy link
Copy Markdown
Contributor Author

3v1n0 commented Dec 11, 2024

Let's close this for now, it can be revisited when adding autopkgtests.

@3v1n0 3v1n0 closed this Dec 11, 2024
adombeck added a commit that referenced this pull request Jan 21, 2026
adombeck added a commit that referenced this pull request Jan 22, 2026
adombeck added a commit that referenced this pull request Jan 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants