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

fix/notification-command-inherited-fields-missing refs #2286 #2764

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

moreamazingnick
Copy link
Contributor

Icinga director has different relations for Checks and Notifications but references the same db-table
for checks: check_command
for notifications: command

IcingaObjectfield loader only checks relation check_command

@cla-bot cla-bot bot added the cla/signed label Jun 12, 2023
@moreamazingnick
Copy link
Contributor Author

seems to be more complicated since the var resolver doesnt involve inheritance from a notification command. but the inheritance works

@moreamazingnick
Copy link
Contributor Author

If we patch protected function eventuallyGetResolvedCommandVar(IcingaObject $object, $varName) ind DIrectorDatafield the resolver works works fine.

@Thomas-Gelf Please tell me if I should prepare another merge request, or if this inheritance behaviour is not a bug but a feature

@lippserd
Copy link
Member

@moreamazingnick can you please reset your branch to master and reapply your change and then do a force push?

@raviks789 please check this PR.

@raviks789
Copy link
Collaborator

@lippserd I am not sure I understand the issue and why this change is required. It would nice if there are screenshots of the issue or steps to reproduce the issue.

@moreamazingnick
Copy link
Contributor Author

@raviks789
If a notification template uses a notificationcommand the command fields are not present like in a servicetemplate
they have to be maually added.

without the patch:

  • create notification template
  • add notificationcommand
  • no fields

with the patch:

  • create notification template
  • add notificationcommand
  • all fields from the notificationcommand fields
    Best regards
    Nicolas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants