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

Implement sending commands via Unix sockets with optional systemd socket activation. #143

Merged
merged 1 commit into from
Aug 26, 2023

Conversation

davnotdev
Copy link
Contributor

This PR addresses #33 and #34. Though, there are a few places that need to be smoothed out:

  1. Logs are done using eprintln. Perhaps I should use the log crate?
  2. The files in systemd/ assume that burrow is installed in /usr/local/bin/burrow, but this is not ideal. As @Muirrum mentioned, those files should be up to distro packagers.
  3. There are a few nasty unwraps I'd like to replace with proper error handling, but I feel like that's outside the scope of this PR.

@davnotdev
Copy link
Contributor Author

davnotdev commented Jul 16, 2023

I merged main into my daemon branch, and now everything is red and failing to compile. Woopsies.
Never mind, I think I've undone it.

@conradev
Copy link
Collaborator

Sorry for taking so long to review this – it looks great!

  1. We should absolutely use logging macros instead of eprintln
  2. Assumptions in those files are okay for now, the service files are really suggestions for the time being
  3. Agreed

There are a few random items:

  • Requests could consider having an ID so you can send multiple over the same connection at once
  • The daemon expects and depends on systemd right now, but ideally the daemon does some runtime discovery to figure out what to do: if the socket is free, grab it, if it isn't, check if systemd has it, else, error

@conradev
Copy link
Collaborator

also the commits do need to be squashed into one

@davnotdev davnotdev force-pushed the daemon branch 2 times, most recently from 67d09c7 to 1cb7e7d Compare August 26, 2023 17:29
@davnotdev davnotdev merged commit f869cbd into main Aug 26, 2023
9 checks passed
@davnotdev davnotdev deleted the daemon branch August 26, 2023 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants