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

Enrich custom variable lists in monitoring and icingadb details #2445

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

Conversation

nilmerg
Copy link
Member

@nilmerg nilmerg commented Dec 2, 2021

Copy link
Contributor

@Thomas-Gelf Thomas-Gelf left a comment

Choose a reason for hiding this comment

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

I really like the motivation behind this feature request and the possibilities this hook provides.

Problems spotted in this pull request: field resolution cannot be solved by joining field names with variable names. It's perfectly legal to configure the very same field multiple times, with different labels, different settings/rules and in different categories. Fields might also be provided via Hook.

This means that walking the template tree in the right order, dealing with fields with filters and fields overriding fields at template level are part of this task.

To avoid duplicate logic, you could leverage IcingaObjectFieldLoader, as it is doing exactly this. I'd suggest to run & populate it, then get the form elements and pick their labels and values. If you need to restructure that class please let me know, I'm sure I can help you saving some time with that task.

@nilmerg nilmerg force-pushed the feature/enrich-cv-lists-in-monitoring-and-icingadb-details-2239 branch from 3b39f2a to 9c9aad0 Compare December 14, 2021 08:07
@nilmerg
Copy link
Member Author

nilmerg commented Dec 14, 2021

I've made use of IcingaObjectFieldLoader now, thanks for this hint. Though, IcingaObjectFieldLoader::getFields() already seems to work fine with the template tree and hooks, no need to access any form elements. 🤔

@nilmerg nilmerg force-pushed the feature/enrich-cv-lists-in-monitoring-and-icingadb-details-2239 branch from 9c9aad0 to 6f66fc9 Compare January 21, 2022 09:13
@nilmerg nilmerg added this to the 1.9.0 milestone Jan 31, 2022
@nilmerg
Copy link
Member Author

nilmerg commented Feb 14, 2022

Please don't merge yet, or give me a hint when you plan to do so. I'm currently playing with an idea to extend this to data exports. (So that your hidden option for strings also works in json and csv exports)

@Thomas-Gelf Thomas-Gelf removed this from the 1.9.0 milestone Feb 17, 2022
@nilmerg nilmerg force-pushed the feature/enrich-cv-lists-in-monitoring-and-icingadb-details-2239 branch from 6f66fc9 to 17a3516 Compare March 2, 2022 10:41
@nilmerg
Copy link
Member Author

nilmerg commented Mar 2, 2022

Please don't merge yet, or give me a hint when you plan to do so. I'm currently playing with an idea to extend this to data exports. (So that your hidden option for strings also works in json and csv exports)

Nope. Changed my mind. Nothing to do here. You can merge, if you are fine with the changes.

@lippserd lippserd added this to the 1.10.0 milestone Apr 8, 2022
@nilmerg nilmerg force-pushed the feature/enrich-cv-lists-in-monitoring-and-icingadb-details-2239 branch from 17a3516 to 9ec08cf Compare May 12, 2022 14:46
@Thomas-Gelf Thomas-Gelf modified the milestones: 1.10.0, next Jun 20, 2022
@Thomas-Gelf
Copy link
Contributor

Hi @nilmerg,

I discussed this thoroughly with @lippserd, there are some problems with this Hook. The variable rendering part is fine, the way variables are iterated is the problem. We already agreed on a slightly different implementation, with only some smaller details left for clarification. We also had a customer interested in pushing the finalization of this, it was part of a deal with a vSphereDB-related feature.

Unfortunately, this hasn't been accepted from our side for unknown reasons, so I have to skip this right now. We'll find a solution, don't worry - but not today.

In the meantime, thank you for your effort!

Cheers,
Thomas

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.

Show Director Labels in Monitoring Module Custom Vars section
3 participants