Skip to content

Conversation

@christianpape
Copy link
Contributor

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR migrates trapd configuration from XML file-based configuration to the Configuration Management (CM) system in the database, supporting the broader effort to centralize configuration management.

Changes:

  • Introduced TrapdConfigDao and DefaultTrapdConfigDao to manage trapd configuration through CM
  • Refactored TrapdConfigFactory to use DAO instead of direct file reading
  • Added test infrastructure with NullEventForwarder and updated test contexts across the codebase

Reviewed changes

Copilot reviewed 109 out of 109 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
opennms-dao-api/src/main/java/org/opennms/netmgt/dao/api/TrapdConfigDao.java Defines DAO interface for trapd configuration
opennms-dao/src/main/java/org/opennms/netmgt/dao/jaxb/DefaultTrapdConfigDao.java Implements DAO using CM service
opennms-config/src/main/java/org/opennms/netmgt/config/TrapdConfigFactory.java Refactored to use DAO instead of file reading
opennms-config-jaxb/src/main/java/org/opennms/netmgt/config/trapd/TrapdConfiguration.java Updated field naming from underscore prefix to camelCase
opennms-config-jaxb/src/main/java/org/opennms/netmgt/config/trapd/Snmpv3User.java Updated field naming from underscore prefix to camelCase
core/test-api/services/src/main/java/org/opennms/netmgt/config/mock/NullEventForwarder.java Added test mock for EventForwarder
features/config/upgrade/src/main/resources/changelog-cm/36.0.0/changelog-cm.xml Added CM schema registration for trapd configuration
opennms-dao/src/main/resources/META-INF/opennms/applicationContext-dao.xml Registered trapd DAO beans and services
Multiple test files Updated test contexts to include nullEventForwarder configuration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

christianpape and others added 2 commits February 4, 2026 10:05
…s/netmgt/threshd/CollectionResourceWrapperIT.java

Co-authored-by: Copilot <[email protected]>
@christianpape christianpape changed the base branch from develop to features/trapd-config-db-migration February 10, 2026 09:52
Copy link
Contributor

@cgorantla cgorantla left a comment

Choose a reason for hiding this comment

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

Good work. I'm surprised how little changed in actual source files.

@@ -0,0 +1,8 @@
package org.opennms.netmgt.dao.api;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit : missing copyright headers

@@ -0,0 +1,42 @@
package org.opennms.netmgt.dao.jaxb;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing copyright headers


@Override
public Consumer<ConfigUpdateInfo> getUpdateCallback(){
return new ConfigurationReloadEventCallback(eventForwarder);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we may need
ConfigurationReloadEventCallback(eventForwarder, this);

ref: 456028c

Comment on lines -81 to 86
TrapdConfigFactory.init();
final TrapdConfigDao trapdConfigDao = BeanUtils.getBean("daoContext", "trapdConfigDao", TrapdConfigDao.class);
TrapdConfigFactory.init(trapdConfigDao);
trapPort = String.valueOf(TrapdConfigFactory.getInstance().getSnmpTrapPort());
} catch (Throwable e) {
// if factory can't be initialized, status is already 'Unknown'
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is worth keeping it just for fetching port

Comment on lines +105 to +119
<bean id="trapdConfigDao" class="org.opennms.netmgt.dao.jaxb.DefaultTrapdConfigDao"/>
<onmsgi:service interface="org.opennms.netmgt.dao.api.TrapdConfigDao" ref="trapdConfigDao" />

<bean id="trapdConfig-init" class="org.springframework.beans.factory.config.MethodInvokingFactoryBean" lazy-init="true">
<property name="staticMethod"><value>org.opennms.netmgt.config.TrapdConfigFactory.init</value></property>
<property name="arguments">
<list>
<ref bean="trapdConfigDao"/>
</list>
</property>
</bean>

<bean id="trapdConfig" class="org.opennms.netmgt.config.TrapdConfigFactory" depends-on="trapdConfig-init" lazy-init="true" factory-method="getInstance"/>

<onmsgi:service interface="org.opennms.netmgt.config.TrapdConfig" ref="trapdConfig" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this to applicationContext-trapd.xml just like DefaultProvisiondConfigurationDao was wired in applicationContext-provisiond.xml.

That will eliminate all the test updates you had to

return Objects.hash(_snmpTrapAddress, _snmpTrapPort, _has_snmpTrapPort, _newSuspectOnTrap, _snmpv3UserList,
_includeRawMessage, _threads, _queueSize, _batchSize, _batchInterval, _useAddessFromVarbind);
return Objects.hash(snmpTrapAddress, snmpTrapPort, hasSnmpTrapPort, newSuspectOnTrap, snmpv3User,
includeRawMessage, threads, queueSize, batchSize, batchInterval, _useAddessFromVarbind);
Copy link
Contributor

Choose a reason for hiding this comment

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

_useAddessFromVarbind should also be renamed to useAddressFromVarbind.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants