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

Log a warning when item for trigger/condition is missing or removed #3007

Merged
merged 2 commits into from
Jul 10, 2022

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Jun 19, 2022

Closes #2526

The solution here not only works for DSL but for all rules since the check is done in the module handler.

Signed-off-by: Jan N. Klug github@klug.nrw

Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K J-N-K added the enhancement An enhancement or new feature of the Core label Jun 19, 2022
@J-N-K J-N-K requested a review from a team as a code owner June 19, 2022 08:25
@kaikreuzer
Copy link
Member

The discussion linked in #2526 suggests that this should be addressed through the VSCode extension as it seems risky to add it to the runtime due to potential issues with the lifecycle of rules and items, especially when editing them at runtime.
Did you consider those concerns here?

@J-N-K
Copy link
Member Author

J-N-K commented Jun 19, 2022

That‘s why I added it here and subscribed to the ItemAddedEvent/ItemRemovedEvent.That way runtime changes to items will be properly considered. And it also works for UI defined rules and all other languages, IMO it‘s superior to VSCode.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

That way runtime changes to items will be properly considered.

I am not sure how this addresses potential lifecycle issues, but I assume you have tested this, so I'm fine here.

So just my wish to slightly improve the log messages remains, then we should be good to merge.

properties.put("event.topics", topic);
eventSubscriberRegistration = this.bundleContext.registerService(EventSubscriber.class.getName(), this,
properties);

if (itemRegistry.get(groupName) == null) {
logger.warn("Group '{}' needed for rule '{}', trigger '{}' not present in registry. Trigger will not work.",
Copy link
Member

Choose a reason for hiding this comment

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

I think the message about the trigger is incorrect/misleading. The trigger is in the registry, the item is not.
I'd suggest to change it to something like, which is anyhow shorter and has all relevant information:

Suggested change
logger.warn("Group '{}' needed for rule '{}', trigger '{}' not present in registry. Trigger will not work.",
logger.warn("Group '{}' needed for rule '{}', Trigger '{}' will not work.",

Same of course for all the other log messages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Signed-off-by: Jan N. Klug <github@klug.nrw>
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Thank you!

@kaikreuzer kaikreuzer merged commit 2ad0b73 into openhab:main Jul 10, 2022
@kaikreuzer kaikreuzer added this to the 3.4 milestone Jul 10, 2022
@J-N-K J-N-K deleted the feature-logmissingitem branch July 18, 2022 16:44
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
…penhab#3007)

* Log a warning when item for trigger/condition is missing or removed

Signed-off-by: Jan N. Klug <github@klug.nrw>
GitOrigin-RevId: 2ad0b73
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Rule DSL] log a WARN if item does not exist
2 participants