[IA-5017] Event pubsub google client code #19
Conversation
service/src/test/java/bio/terra/appmanager/dao/PublisherDaoTest.java
Outdated
Show resolved
Hide resolved
service/src/main/java/bio/terra/appmanager/config/PublisherConfiguration.java
Outdated
Show resolved
Hide resolved
service/src/test/java/bio/terra/appmanager/dao/PublisherDaoTest.java
Outdated
Show resolved
Hide resolved
service/src/test/java/bio/terra/appmanager/service/ChartServiceTest.java
Show resolved
Hide resolved
| @@ -67,6 +70,8 @@ void testCreateChart_singleElement() { | |||
| assertEquals(chart1.name(), argument.getValue().name()); | |||
| assertEquals(chart1.version(), argument.getValue().version()); | |||
| assertNull(argument.getValue().activeAt()); | |||
|
|
|||
| verify(publisherDao, times(1)).publish("chart created"); | |||
There was a problem hiding this comment.
I am thinking through the additional verifications on this test ... curious to have the brainstorm (maybe) here.
There is the validating that things are being created (chartDao) and that appropriate events are being fired. Maybe need to think through making this clearer? Meaning, does it make sense to break out the verifications into separate explicit methods such that they are clear on what we are validating? maybe something like #verifyChart and #verifyEvent
service/src/main/java/bio/terra/appmanager/config/PubsubConfig.java
Outdated
Show resolved
Hide resolved
service/src/main/java/bio/terra/appmanager/dao/PublisherDao.java
Outdated
Show resolved
Hide resolved
| implementation platform('com.google.cloud:libraries-bom:26.42.0') | ||
| implementation 'com.google.cloud:google-cloud-pubsub' | ||
|
|
||
| implementation platform('org.testcontainers:testcontainers-bom:1.20.1') |
There was a problem hiding this comment.
Needed for pubsub emulator and unit testing
service/src/main/java/bio/terra/common/events/client/google/GooglePubsubClient.java
Show resolved
Hide resolved
| } | ||
| return builder.build(); | ||
| } catch (IOException | ConfigurationException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Generic Java term that we unfortunately overloaded in leo 😓
| ApplicationContext applicationContext, | ||
| BeeConfig beeConfig, | ||
| GoogleConfig googleConfig, | ||
| ObjectProvider<String> pubsubEmulatorEndpoint) { |
There was a problem hiding this comment.
Could you leave a comment explaining what the pubsubEmulatorEndpoint does and how it gets injected here?
| return process(msg); | ||
| } catch (JsonProcessingException e) { | ||
| logger.error("error while converting message data to EventMessage", e); | ||
| // TODO: what to do with bad messages |
There was a problem hiding this comment.
Should be taken care of already by the dead letter queue
| * This is the current version of the EventMessage schema, as defined in the spec (see class-level | ||
| * javadoc). | ||
| */ | ||
| public static String MESSAGE_VERSION = "1.0.0"; |
There was a problem hiding this comment.
Can you add a comment linking the chage in terraform that needs to be done if the schema needs to be updated?
| @@ -0,0 +1,7 @@ | |||
| package bio.terra.common.events.topics.messages; | |||
|
|
|||
| public enum EventTypes { | |||
There was a problem hiding this comment.
Maybe leave a warning about preserving the order of this enum list as it can get messy quickly
There was a problem hiding this comment.
Would a dict with a key/value pair bring more robustness?
| @@ -2,6 +2,14 @@ | |||
| # This is for deployment-specific values, which may be managed by other teams | |||
|
|
|||
| env: | |||
| google: | |||
| project_id: ${SERVICE_GOOGLE_PROJECT:broad-dsde-dev} | |||
There was a problem hiding this comment.
Could you also add pubsub emulator for dummies section in the contributing.md? ;)
| public abstract class BaseEventsTest<T extends EventTopic<?>> extends BaseSpringBootTest { | ||
|
|
||
| @TestConfiguration | ||
| public static class MockEventConfiguration<T> { |
There was a problem hiding this comment.
Could this be MockChartEventConfiguration?
|
No description provided.