-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Migrate spring data cosmos modules to sdk/spring dir #30854
Migrate spring data cosmos modules to sdk/spring dir #30854
Conversation
API change check APIView has identified API level changes in this PR and created following API reviews. azure-spring-data-cosmos |
/azp run java - spring - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
The below failures will be fixed later:
|
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.
Changes to CODEOWNERS and Fabric Bot rules look good to me.
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.
@moarychan great work, thanks!
I have added few comments which are worth discussing before we merge this PR.
Regarding your comment on Cosmos IT failures, which may be fixed in main branch first.
There have been no failures regarding CosmosAuditing feature in our CI, and I think the code changes introduced in this PR related to auditing feature are causing the pipeline to fail. Please fix the tests before merging this PR.
@@ -143,8 +143,7 @@ com.azure:azure-security-keyvault-perf;1.0.0-beta.1;1.0.0-beta.1 | |||
com.azure:azure-sdk-template;1.1.1234;1.2.2-beta.1 | |||
com.azure:azure-sdk-template-two;1.0.0-beta.1;1.0.0-beta.1 | |||
com.azure:azure-sdk-template-three;1.0.0-beta.1;1.0.0-beta.1 | |||
com.azure:azure-spring-data-cosmos;3.26.0;3.27.0-beta.1 | |||
com.azure:azure-spring-data-cosmos-test;3.0.0-beta.1;3.0.0-beta.1 | |||
com.azure:azure-spring-data-cosmos;6.0.0-beta.1;6.0.0-beta.1 |
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.
current versions of azure-spring-data-cosmos is v3. Why do we have the version as 6.0.0-beta.1 ?
What is the plan for releasing multiple major versions of azure-spring-data-cosmos?
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.
We can discuss the name of the version in the tmr meeting. The release plan is current milestone 2022-10, after this pr is merged, it will be very soon.
@@ -36,29 +36,3 @@ stages: | |||
TestGoals: 'verify' | |||
TestOptions: '$(ProfileFlag) $(AdditionalArgs)' | |||
TestResultsFiles: '**/junitreports/TEST-*.xml' | |||
|
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.
Why are we removing this? Can't we still run spring-data-cosmos-test integration pipelines on azure-cosmos
module development?
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.
+1 - same comment for the ci.yml above. We would still need to compile and run the spring-data-cosmos tests for any changes in azure-cosmos
<dependency> | ||
<groupId>com.azure</groupId> | ||
<artifactId>azure-cosmos</artifactId> | ||
<version>4.36.0-beta.1</version> <!-- {x-version-update;com.azure:azure-cosmos;current} --> | ||
<version>4.35.0</version> <!-- {x-version-update;com.azure:azure-cosmos;dependency} --> |
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.
We should still depend on current version of azure-cosmos like we do right now, so that we can continuously run the integration tests when we develop azure-cosmos module.
If we don't do this, we won't be able to catch any regressions because of development on azure-cosmos module, which can then only be caught after the azure-cosmos module has been released.
@@ -85,10 +85,21 @@ | |||
<artifactId>spring-expression</artifactId> | |||
<version>6.0.0-M5</version> <!-- {x-version-update;org.springframework:spring-expression;external_dependency} --> | |||
</dependency> | |||
<dependency> | |||
<groupId>io.micrometer</groupId> |
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.
why do we need micrometer in spring-data-cosmos?
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.
It's used to fix the issue micrometer-metrics/micrometer#3398
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.
But shouldn't it be sufficient to add this as "provided" vs. making it a transitive dependency?
@@ -123,7 +123,7 @@ or contact [opencode@microsoft.com][coc_contact] with any additional questions o | |||
[coc_faq]: https://opensource.microsoft.com/codeofconduct/faq/ | |||
[coc_contact]: mailto:opencode@microsoft.com | |||
[azure_subscription]: https://azure.microsoft.com/free/ | |||
[samples]: https://github.com/Azure/azure-sdk-for-java/tree/main/sdk/cosmos/azure-spring-data-cosmos/src/samples/java/com/azure/spring/data/cosmos | |||
[samples]: https://github.com/Azure/azure-sdk-for-java/tree/feature/spring-cloud-azure-6.x/sdk/spring/azure-spring-data-cosmos/src/samples/java/com/azure/spring/data/cosmos |
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.
Shouldn't we stick with the Main branch here? My assumption was the feature branch is used for future versions - while Main contains the current stable version?
https://github.com/Azure/azure-sdk-for-java/tree/main/sdk/spring/azure-spring-data-cosmos/
@@ -122,7 +122,7 @@ or contact [opencode@microsoft.com][coc_contact] with any additional questions o | |||
[coc_faq]: https://opensource.microsoft.com/codeofconduct/faq/ | |||
[coc_contact]: mailto:opencode@microsoft.com | |||
[azure_subscription]: https://azure.microsoft.com/free/ | |||
[samples]: https://github.com/Azure/azure-sdk-for-java/tree/main/sdk/cosmos/azure-spring-data-cosmos/src/samples/java/com/azure/spring/data/cosmos | |||
[samples]: https://github.com/Azure/azure-sdk-for-java/tree/feature/spring-cloud-azure-6.x/sdk/spring/azure-spring-data-cosmos/src/samples/java/com/azure/spring/data/cosmos |
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.
See above - stick with Main branch
@@ -143,8 +143,7 @@ com.azure:azure-security-keyvault-perf;1.0.0-beta.1;1.0.0-beta.1 | |||
com.azure:azure-sdk-template;1.1.1234;1.2.2-beta.1 | |||
com.azure:azure-sdk-template-two;1.0.0-beta.1;1.0.0-beta.1 | |||
com.azure:azure-sdk-template-three;1.0.0-beta.1;1.0.0-beta.1 | |||
com.azure:azure-spring-data-cosmos;3.26.0;3.27.0-beta.1 | |||
com.azure:azure-spring-data-cosmos-test;3.0.0-beta.1;3.0.0-beta.1 | |||
com.azure:azure-spring-data-cosmos;6.0.0-beta.1;6.0.0-beta.1 |
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.
Why make this look like a breaking change (with major verson bump)?
<packaging>jar</packaging> | ||
<name>Spring Data Test for Azure Cosmos DB SQL API</name> | ||
<description>Spring Data Test for Azure Cosmos DB SQL API</description> | ||
<url>https://github.com/Azure/azure-sdk-for-java/tree/main/sdk/cosmos/azure-spring-data-cosmos-test</url> | ||
<url>https://github.com/Azure/azure-sdk-for-java/tree/feature/spring-cloud-azure-6.x/sdk/spring/azure-spring-data-cosmos-test</url> |
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.
See above - should stick with main
import org.springframework.stereotype.Repository; | ||
|
||
@Repository | ||
public interface PageableAddressRepository extends PagingAndSortingRepository<Address, String> { | ||
public interface PageableAddressRepository extends CrudRepository<Address, String>, PagingAndSortingRepository<Address, String> { |
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.
Please revert = PagingAndSortingRepository already extends CrudRepository - so this change isn't necessary
import org.springframework.data.repository.PagingAndSortingRepository; | ||
import org.springframework.stereotype.Repository; | ||
|
||
@Repository | ||
public interface PageableMemoRepository extends PagingAndSortingRepository<PageableMemo, String> { | ||
public interface PageableMemoRepository extends CrudRepository<PageableMemo, String>, PagingAndSortingRepository<PageableMemo, String> { |
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.
See above -please revert
import org.springframework.data.repository.PagingAndSortingRepository; | ||
import org.springframework.stereotype.Repository; | ||
|
||
@Repository | ||
public interface PageablePersonRepository extends PagingAndSortingRepository<PageablePerson, String> { | ||
public interface PageablePersonRepository extends CrudRepository<PageablePerson, String>, PagingAndSortingRepository<PageablePerson, String> { |
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.
see above - pelase revert
@@ -618,7 +618,7 @@ azure.cosmos.secondary.database=your-secondary-cosmosDb-dbName | |||
azure.cosmos.secondary.populateQueryMetrics=if-populate-query-metrics | |||
``` | |||
|
|||
- The [Entity](https://github.com/Azure/azure-sdk-for-java/tree/main/sdk/cosmos/azure-spring-data-cosmos#define-an-entity) and [Repository](https://github.com/Azure/azure-sdk-for-java/tree/main/sdk/cosmos/azure-spring-data-cosmos#create-repositories) definition is similar as above. You can put different database entities into different packages. | |||
- The [Entity](https://github.com/Azure/azure-sdk-for-java/tree/feature/spring-cloud-azure-6.x/sdk/spring/azure-spring-data-cosmos#define-an-entity) and [Repository](https://github.com/Azure/azure-sdk-for-java/tree/feature/spring-cloud-azure-6.x/sdk/spring/azure-spring-data-cosmos#create-repositories) definition is similar as above. You can put different database entities into different packages. |
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.
please stick to main
</parent> | ||
|
||
<groupId>com.azure</groupId> | ||
<artifactId>azure-spring-data-cosmos</artifactId> | ||
<version>3.27.0-beta.1</version> <!-- {x-version-update;com.azure:azure-spring-data-cosmos;current} --> | ||
<version>6.0.0-beta.1</version> <!-- {x-version-update;com.azure:azure-spring-data-cosmos;current} --> |
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.
Not clear why this would be marked as breaking change (major version upgrade)? We probably need to discuss in a meeting - anytime we introduce a new major version we need to have a plan to deprecate/retire the previous one - and understand that we duplicate support cost.
@@ -15,7 +16,7 @@ | |||
* pagination and sorting abstraction. | |||
*/ | |||
@NoRepositoryBean | |||
public interface CosmosRepository<T, ID extends Serializable> extends PagingAndSortingRepository<T, ID> { | |||
public interface CosmosRepository<T, ID extends Serializable> extends CrudRepository<T, ID>, PagingAndSortingRepository<T, ID> { |
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.
Please revert - not necessary PagingAndSortedRepository already extends CrudRepository - so CosmosRepository is implicitly extending CrudRepository already
import org.springframework.data.repository.reactive.ReactiveSortingRepository; | ||
import reactor.core.publisher.Flux; | ||
import reactor.core.publisher.Mono; | ||
/** | ||
* Repository interface with search and delete operation | ||
*/ | ||
@NoRepositoryBean | ||
public interface ReactiveCosmosRepository<T, K> extends ReactiveSortingRepository<T, K> { | ||
public interface ReactiveCosmosRepository<T, K> extends ReactiveCrudRepository<T, K>, ReactiveSortingRepository<T, K> { |
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.
Dito
@@ -6,7 +6,7 @@ | |||
"ProfileFlag": "-P integration-test-azure", | |||
"Pool": "azsdk-pool-mms-ubuntu-2004-general", | |||
"OSVmImage": "MMSUbuntu2004", | |||
"JavaTestVersion": "1.11" | |||
"JavaTestVersion": "1.17" |
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 guess adding Java 17 test coverage is goodness - but removing Java 11 test coverage is bad. Vast majority of our customers still uses Java 8 or 11
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.
+1 to Fabian. Also, Java 17 doesn't work with spring-data-cosmos because of UUID support as Id type. There is an open bug #28926
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.
Few comments - also would like to understand the game plan on how to execute the move from sdk/cosmos to sdk/spring in main branch. My preference would have been to do the move in main first - to minimize the time where possibly new dev work / feature work is done in the feature branch and code diverges between master and feature branch more and more.
Close this one, new PR is comming. |
Description
Fixes #30793 #30458
If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines