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

Remove cosmos permissions properties and set default connection mode #27236

Conversation

saragluna
Copy link
Member

@saragluna saragluna commented Feb 22, 2022

Per discussion with the architects:

cosmos.gateway-connection.proxy and spring.cloud.azure.cosmos.permissions are complex types, property is defined, but nested properties are not defined. there is no metadata and there is no way to populate them via config. They should not be listed as properties considering this.

This is not just a metadata concern, users are not able to configure corresponding SDK options type in application.yml.
There should be some tests that validate how each configuration is applied if it's exposed at all. Suggestion is to remove 'permissions' property until it can be supported and tested

cosmos.connection-mode is null by default, meaning this code block does nothing and gateway or direct configs are ignored. it should follow SDK default (direct) or clearly communicate in docs that property is required (and log error if it's not provided)

#27112

@ghost ghost added the azure-spring All azure-spring related issues label Feb 22, 2022
@saragluna saragluna changed the title Remove cosmos permissions properties Remove cosmos permissions properties and set default connection mode Feb 22, 2022
@saragluna saragluna force-pushed the xiada/fix/remove-cosmos-permissions-properties branch from ce17d3e to 74f492a Compare February 22, 2022 09:09
@saragluna saragluna force-pushed the xiada/fix/remove-cosmos-permissions-properties branch from 74f492a to c22b640 Compare February 22, 2022 09:43
/**
* Permission list which contains the resource tokens needed to access resources.
*/
private final List<CosmosPermissionProperties> permissions = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

The test properties should be removed in sync,

Copy link
Member Author

Choose a reason for hiding this comment

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

cool, thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do in another PR.

@@ -97,7 +92,7 @@
/**
* Connection mode to be used by the client in the Azure Cosmos DB database service.
*/
private ConnectionMode connectionMode;
private ConnectionMode connectionMode = ConnectionMode.DIRECT;
Copy link
Member

Choose a reason for hiding this comment

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

The test properties should be updated in sync, com.azure.spring.service.implementation.cosmos.TestAzureCosmosHttpProperties#connectionMode

Copy link
Contributor

@backwind1233 backwind1233 left a comment

Choose a reason for hiding this comment

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

LGTM

@saragluna saragluna merged commit 1208566 into Azure:feature/azure-spring-cloud-4.0 Feb 23, 2022
@saragluna saragluna self-assigned this Feb 28, 2022
@saragluna saragluna deleted the xiada/fix/remove-cosmos-permissions-properties branch March 17, 2022 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
azure-spring All azure-spring related issues
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants