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.

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

Feature/migrate-spring-data-cosmos #12287

Merged
merged 17 commits into from
Jun 25, 2020

Conversation

yiliuTo
Copy link
Member

@yiliuTo yiliuTo commented Jun 17, 2020

Migrate source code and unit tests from repo spring-data-cosmos here.

Change the dependency in azure-spring-boot of spring-data-cosmos from external to current.

Keep several dependency versions different from SDK including

  • spring_org.springframework:spring-web
  • spring_com.microsoft.azure:azure-cosmos
  • spring_org.springframework.boot:spring-boot-starter-test

<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-identity</artifactId>
<version>1.0.4</version>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be using the latest GA version available which is 1.0.7?

Copy link
Member Author

@yiliuTo yiliuTo Jun 18, 2020

Choose a reason for hiding this comment

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

Yes, this snippet failed to be modified by rebasing and I will delete it manually.

Write the repository interface:
```
@Repository
public interface AddressRepository extends DocumentDbRepository<Address, String> {
Copy link
Member

Choose a reason for hiding this comment

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

There is no DocumentDbRepository. That is in old spring-data-cosmosdb SDK.
This should extend CosmosRepository or ReactiveCosmosRepository

@@ -0,0 +1,58 @@
### How to Query Partitioned Azure Cosmos DB Collection
Copy link
Member

Choose a reason for hiding this comment

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

What is the use of this file ?
This should be merged with README file, or move this topic to spring-data-cosmosdb developer's guide : https://docs.microsoft.com/en-us/azure/developer/java/spring-framework/how-to-guides-spring-data-cosmosdb

Copy link
Member Author

Choose a reason for hiding this comment

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

The content of this file has been included into README file about using partitioned query, thus I will remove this file.

Copy link
Member

@JimSuplizio JimSuplizio left a comment

Choose a reason for hiding this comment

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

Right now I'm only making a few comments about the external dependencies. Before I review anything else I expect the Analyze, specifically the pom file version scanner step, and the From Source runs to be passing. There a are a number of errors that the version scanner picked up that are listed in the output that need to be fixed.

#sdk\spring\spring-data-cosmosdb\pom.xml
spring_org.springframework:spring-web;5.2.6.RELEASE
spring_com.microsoft.azure:azure-cosmos;3.7.3
spring_org.springframework.boot:spring-boot-starter-test;2.3.0.RELEASE
Copy link
Member

Choose a reason for hiding this comment

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

First, spring_ only dependencies should be infrequent or in rare cases. The org.springframework versions here have been updated as part of another PR which has been approved. I recommended rebasing after that gets submitted. Second, the com.microsoft.azure:azure-cosmos dependency was added as part of this Spring PR several weeks back and apparently nothing previously used it. So the spring_com.microsoft.azure:azure-cosmos is unnecessary and the existing dependency version should be updated. In the future, I'm like to recommend either grep/findstr/search within whatever IDE is being used. If a dependency is an existing one, then updating that is the preferred method over declaring spring specific dependencies which may require contacting another library owner and those can be found in the CODEOWNERS file

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestions. I have rebased the related PR and updated version of the dependencies.

@yiliuTo yiliuTo force-pushed the feature/migrate-spring-data-cosmos branch from d5bd3ac to 1a0201e Compare June 18, 2020 02:25
@yiliuTo yiliuTo requested a review from srnagar as a code owner June 18, 2020 02:25
@yiliuTo
Copy link
Member Author

yiliuTo commented Jun 18, 2020

Hi @JimSuplizio, due to the -amd parameter in java-core-ci pipeline, the azure-spring-boot project gets triggered while spring-data-cosmosdb not, however azure-spring-boot depends on spring-data-cosmosdb thus the java-core-ci pipeline always fails.
To deal with this problem, currently I add a useless dependency azure-identity in spring-data-cosmos and now the pipeline can pass now. It seems such solution doesn't solve the problem completely and imports a useless dependency. Could you please help us with some suggestions? Thank you~

@yiliuTo yiliuTo force-pushed the feature/migrate-spring-data-cosmos branch from 0e022d7 to ce265b7 Compare June 22, 2020 05:53
@yiliuTo yiliuTo force-pushed the feature/migrate-spring-data-cosmos branch from c659f98 to e2b8ba1 Compare June 22, 2020 08:18
@yiliuTo
Copy link
Member Author

yiliuTo commented Jun 22, 2020

In order to pass the java-core-ci pipeline, currently I add the spring-data-cosmosdb as the external dependency for azure-spring-boot project given that it cannot be built as a dependency of track 2.

@saragluna
Copy link
Member

Hi @JimSuplizio and @kushagraThapar, could you please take a look at this PR?

@JimSuplizio
Copy link
Member

In order to pass the java-core-ci pipeline, currently I add the spring-data-cosmosdb as the external dependency for azure-spring-boot project given that it cannot be built as a dependency of track 2.

This is exactly what I'd said [needed to get done] (Azure/azure-sdk#892 (comment)). Until the spring-data-cosmosdb, being added to the repository, is updated to target v4 cosmos, spring is going to have to use the released versions as external dependencies.

Copy link
Member

@JimSuplizio JimSuplizio left a comment

Choose a reason for hiding this comment

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

I'm approving this as per a discussion between @kushagraThapar and myself. He's going to create work item to make spring-data-comos use v4 cosmos, update the groupId, artifactId and version to reflect these changes. I would still like his approval before this is merged.

@@ -55,7 +55,7 @@ com.microsoft.azure:azure-servicebus-jms-spring-boot-starter;2.3.2;2.3.3-beta.1
com.microsoft.azure:azure-spring-boot-metrics-starter;2.3.2;2.3.3-beta.1
com.microsoft.azure:azure-spring-boot-tests;2.3.2;2.3.3-beta.1
com.microsoft.azure:azure-spring-boot-test-core;2.3.2;2.3.3-beta.1

com.microsoft.azure:spring-data-cosmosdb;2.3.0;2.3.1-beta.1
Copy link
Member

Choose a reason for hiding this comment

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

@kushagraThapar, as per our discussion earlier we'll let this get checked in like this but part of the v4 updates will be to rename the groupId and change the artifactId to match the naming convention. Also, the work item you're creating will also encompass updating the versions to match this update to v4 cosmos which will update the major to 3.

Copy link
Member

Choose a reason for hiding this comment

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

@JimSuplizio - Sounds good. Here is the issue : #12490

@saragluna saragluna merged commit 35d9154 into Azure:master Jun 25, 2020
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.

5 participants