-
Notifications
You must be signed in to change notification settings - Fork 6
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
send situations to the datalake when accepted, add alarm(s), remove a… #100
Conversation
…larm(s) add unit test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look fine. Comments are mostly related to SituationClient impl.
features/ui/src/main/java/org/opennms/alec/rest/SituationRestImpl.java
Outdated
Show resolved
Hide resolved
features/ui/src/main/java/org/opennms/alec/grpc/SituationClient.java
Outdated
Show resolved
Hide resolved
features/ui/src/main/java/org/opennms/alec/grpc/SituationClient.java
Outdated
Show resolved
Hide resolved
features/ui/src/main/java/org/opennms/alec/grpc/SituationClient.java
Outdated
Show resolved
Hide resolved
features/ui/src/main/java/org/opennms/alec/grpc/SituationClient.java
Outdated
Show resolved
Hide resolved
initialize mapper in SituationClient sendSituation can return void initialize channel only if the user opted-in
update guava version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this review before it was merged but forgot to submit it. Figured I'd submit it now to get the comments out there.
<dependency> | ||
<groupId>com.google.guava</groupId> | ||
<artifactId>guava</artifactId> | ||
<version>30.1.1-jre</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should pull this version number up into a property or into dependencyManagement.
client = new SituationClient(canWeStore(kvStore), grpcConnectionConfig); | ||
} | ||
|
||
//Only for testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this constructor only exists so we can mock SituationClient
in the unit tests. Does it make sense for SituationClient
to be converted into a bean, and injected directly? That would probably be cleaner, but it would need some refactors and I'm not sure about the implications regarding the number of SituationClients we need to use.
send situations to the datalake when situation is created,accepted or modified
also fix feedback for rejected situation
add unit test
use async client