Skip to content
This repository was archived by the owner on Feb 16, 2025. It is now read-only.

[new-backend] notification send hooks #4504

Merged

Conversation

atmaxinger
Copy link
Contributor

@atmaxinger atmaxinger commented Oct 3, 2022

Basics

  • Short descriptions of your changes are in the release notes
    (added as entry in doc/news/_preparation_next_release.md which
    contains _(my name)_)
    Please always add something to the release notes.
  • Details of what you changed are in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, are in the commit messages.
  • The buildservers are happy. If not, fix in this order:
    • add a line in doc/news/_preparation_next_release.md
    • reformat the code with scripts/dev/reformat-all
    • make all unit tests pass
    • fix all memleaks
  • The PR is rebased with current master.

Checklist

  • I added unit tests for my code
  • I fully described what my PR does in the documentation
    (not in the PR description)
  • I fixed all affected documentation
  • I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
  • I updated all meta data (e.g. README.md of plugins and METADATA.ini)
  • I mentioned every code not directly written by me in reuse syntax

Review

Labels

  • Add the "work in progress" label if you do not want the PR to be reviewed yet.
  • Add the "ready to merge" label if the basics are fulfilled and no further pushes are planned by you.

@atmaxinger
Copy link
Contributor Author

For the send notification hooks I took a bit of a different approach: As there might be multiple valid choices for plugins to notify, e.g. dbus, zeromq, record, ... I made it in form of a list.

Multiple plugins can be configured using the array system:/elektra/hook/notification/send/plugins, e.g.

system:/elektra/hook/notification/send/plugins/#0 = dbus
system:/elektra/hook/notification/send/plugins/#1 = record

@markus2330 @kodebach any objections to this?

Copy link
Member

@kodebach kodebach left a comment

Choose a reason for hiding this comment

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

The general concept looks good, but I have some notes on the details

@kodebach kodebach mentioned this pull request Oct 5, 2022
25 tasks
Copy link
Member

@kodebach kodebach left a comment

Choose a reason for hiding this comment

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

LGTM now, great work, IMO this could be merged now (unless you want to do something more).

The remaining discussion about using different KeySets with one KDB can be moved to a separate issue, since it is a problem with the current solution (even on master) anyway.

atmaxinger and others added 4 commits October 6, 2022 12:34
Co-authored-by: Klemens Böswirth <23529132+kodebach@users.noreply.github.com>
Co-authored-by: Klemens Böswirth <23529132+kodebach@users.noreply.github.com>
Co-authored-by: Klemens Böswirth <23529132+kodebach@users.noreply.github.com>
@atmaxinger
Copy link
Contributor Author

No, I think this hook is done. The next thing I'll probably tackle would actually be the tracking of the changes, as this is what I also need for the record plugin.

@markus2330
Copy link
Contributor

It is a bit difficult to review for me as I don't know the changes in new-backend enough. @kodebach its best when you decide if this brings us closer to merge new-backend. I'll directly review new-backend, then.

@atmaxinger can you add some docu about how the hooks work, when they are called etc.. Or is it a straightforward implementation of the decision?

@kodebach
Copy link
Member

kodebach commented Oct 6, 2022

kodebach its best when you decide if this brings us closer to merge new-backend. I'll directly review new-backend, then.

IMO we can merge this PR now.

can you add some docu about how the hooks work, when they are called etc..

See https://github.com/ElektraInitiative/libelektra/blob/new-backend/doc/dev/hooks.md

We need more docs on the individual hooks. But I think this can wait until we finalize the entire design (i.e. all planned hooks work they way we want). There is no point in adding docs that might change again soon, if there is no consumer of the docs.

For example, it would be really stupid to document the way spec is called right now, when it isn't clear yet whether we will change this before the next release.

@atmaxinger
Copy link
Contributor Author

I will add some documentation to the doc/dev/hooks.md.

@atmaxinger
Copy link
Contributor Author

@markus2330 @kodebach I added some documentation for all of the current hooks. We can merge this PR now.

@kodebach kodebach merged commit 4e7d195 into ElektraInitiative:new-backend Oct 7, 2022
@kodebach
Copy link
Member

kodebach commented Oct 7, 2022

@markus2330 I merged this PR, if you still want to review the changes do so in #4187.

@mpranj mpranj added this to the 0.9.12 milestone Jan 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants