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.

Sign up for GitHub

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

Jump to bottom

Add autoconfiguration for the ComsosTokenStore #3

Merged
merged 5 commits into from
Feb 2, 2024
Merged

Add autoconfiguration for the ComsosTokenStore #3

merged 5 commits into from
Feb 2, 2024

Conversation

gklijs
Copy link
Contributor

@gklijs gklijs commented Jan 9, 2024

No description provided.

@gklijs gklijs added Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. Status: In Progress Use to signal this issue is actively worked on. Type: Enhancement Use to signal an issue enhances an already existing feature of the project. labels Jan 9, 2024
@gklijs gklijs self-assigned this Jan 9, 2024
@gklijs gklijs force-pushed the feature/autoconfiguration branch from 5f0886b to 7bb7bf1 Compare January 10, 2024 12:15
@smcvb smcvb changed the title Add autoconfiguration. Add autoconfiguration for the ComsosTokenStore Jan 11, 2024
smcvb
smcvb approved these changes Jan 11, 2024
View reviewed changes
Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

My biggest pain point is not having a unit test for the auto-configuration class. Other than that, my comments are just small fry I trust you will pick up correctly.

axon-cosmosdb-spring-boot-3-integrationtests/pom.xml Show resolved Hide resolved
axon-cosmosdb-spring-boot-3-integrationtests/pom.xml Show resolved Hide resolved
axon-cosmosdb-spring-boot-autoconfigure/src/test/resources/log4j2.properties Show resolved Hide resolved
...mework/extensions/cosmosdb/springboot/autoconfig/CosmosAutoConfigurationIntegrationTest.java
import static org.junit.jupiter.api.Assertions.*;

@Testcontainers
class CosmosAutoConfigurationIntegrationTest {
Copy link
Member

Choose a reason for hiding this comment

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

Although I understand that testing the CosmosTokenStore through a unit test didn't work ideal, we can still make a unit test for our auto-configuration. Especially with an eye on the fact the CosmosDBEmulatorContainer doesn't work on GitHub, I think this would be beneficial.

Copy link
Member

Choose a reason for hiding this comment

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

Do we even need an integration test if the test is to check whether a bean is constructed with the right components?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can we unit test it without a client?

Copy link
Member

Choose a reason for hiding this comment

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

Can you perhaps make a NoOp version of the client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, since it's both a final class, and doesn't use an interface (besides Closable).

Copy link
Member

Choose a reason for hiding this comment

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

Boooh, lame :-(
I dislike this inflexibility.

...java/org/axonframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenItemTest.java Show resolved Hide resolved
...nframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenStoreIntegrationTest.java Show resolved Hide resolved
@gklijs
Copy link
Contributor Author

gklijs commented Jan 11, 2024

There is some activity on the repo of the docker image, so I have good hopes it's fixed soon.

@gklijs gklijs force-pushed the feature/autoconfiguration branch from e4a33a1 to b0a7f72 Compare January 19, 2024 11:27
@gklijs gklijs force-pushed the feature/autoconfiguration branch from b0a7f72 to b3dd5da Compare February 2, 2024 08:33
Base automatically changed from feature/token-store to main February 2, 2024 09:14
@gklijs gklijs force-pushed the feature/autoconfiguration branch from b3dd5da to c7aa962 Compare February 2, 2024 09:38
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Feb 2, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@gklijs gklijs merged commit 0d26585 into main Feb 2, 2024
6 of 7 checks passed
@smcvb smcvb deleted the feature/autoconfiguration branch February 2, 2024 09:55
@smcvb smcvb added Status: Resolved Use to signal that work on this issue is done. and removed Status: In Progress Use to signal this issue is actively worked on. labels Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smcvb smcvb smcvb approved these changes

@CodeDrivenMitch CodeDrivenMitch Awaiting requested review from CodeDrivenMitch

Assignees

@gklijs gklijs

Labels
Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. Status: Resolved Use to signal that work on this issue is done. Type: Enhancement Use to signal an issue enhances an already existing feature of the project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gklijs @smcvb