Skip to content

Conversation

@artur-ciocanu
Copy link
Contributor

Description

This is a PR similar to #1089 but it also incorporates changes from #1097 primarily to ensure that integration tests based on Testcontainers are working as expected.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #[issue number]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@artur-ciocanu artur-ciocanu requested review from a team as code owners August 13, 2024 21:25
@artur-ciocanu artur-ciocanu force-pushed the gh-1088-props-override branch from 6ed04c0 to 1458d00 Compare August 14, 2024 10:13
@artur-ciocanu artur-ciocanu force-pushed the gh-1088-props-override branch from 6983aba to 7dd8f9b Compare August 14, 2024 19:38

@BeforeEach
public void waitSetup() {
daprClient.waitForSidecar(1000).block();
Copy link
Collaborator

@salaboy salaboy Aug 15, 2024

Choose a reason for hiding this comment

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

@artursouza we are doing this now, but I am seriously thinking to move this down into the DaprKeyValueTemplate so the user doesn't need to worry about handling the daprClient would it make sense to run daprClient.waitForSidecar(1000).block() before each Dapr API operation? Is that the intended use for all the operations? My gut tells me that it will be inefficient

.withExposedPorts(5432)
.withNetwork(DAPR_NETWORK);

private static final WaitStrategy DAPR_CONTAINER_WAIT_STRATEGY = Wait.forHttp("/v1.0/healthz")
Copy link
Collaborator

Choose a reason for hiding this comment

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

@artur-ciocanu maybe we can make this internal.. meaning that this can be the default strategy. If I remember correctly, depending the parameters that we set we might run into a loop of daprd waiting for the app port to be exposed and the test waiting for daprd to be ready. But as a user I don't want to be dealing with this.

.withPropertyOverride(Properties.HTTP_PORT, String.valueOf(DAPR_CONTAINER.getHttpPort()))
.withPropertyOverride(Properties.GRPC_PORT, String.valueOf(DAPR_CONTAINER.getGrpcPort()));
try (DaprClient client = (builder).build()) {
client.waitForSidecar(5000).block();
Copy link
Collaborator

Choose a reason for hiding this comment

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

@artur-ciocanu can we just use 1s all over the place? If we are waiting for more than 1 second then something is wrong somewhere else.


@Configuration
@EnableConfigurationProperties(DaprPubSubProperties.class)
static class DartSpringMessagingConfiguration {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@artur-ciocanu fix typo in the class name

<groupId>io.dapr.spring</groupId>
<artifactId>dapr-spring-data</artifactId>
<version>${project.parent.version}</version>
<optional>true</optional>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@artur-ciocanu if these are optional, then, as a user we don't get these dependencies if we include the dapr-spring-boot-starter which means that we need to separately add this dependencies manually.. was there a reason why this needs to be optional?

import java.util.List;
import java.util.Map;

import static io.dapr.it.spring.data.DaprSpringDataConstants.BINDING_NAME;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@artur-ciocanu do we want to make this user facing constants? This should be part of the DaprPubSub and DaprStatestore properties no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@artur-ciocanu the binding and the statestore name should have default values here: DaprStateStoreProperties

@cicoyle
Copy link
Contributor

cicoyle commented Aug 26, 2024

Mind resolving the conflicts here :)

@cicoyle cicoyle added this to the v1.13 milestone Aug 26, 2024
@artur-ciocanu
Copy link
Contributor Author

This is no longer required. All the changes have been ported to #1089.

@artur-ciocanu artur-ciocanu deleted the gh-1088-props-override branch August 27, 2024 11:45
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.

3 participants