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

Certification test for Azure CosmosDB Statestore #1972

Merged
merged 4 commits into from Sep 8, 2022

Conversation

shivamkm07
Copy link
Contributor

Signed-off-by: shivam shivamkm07@gmail.com

Description

Certification test for Azure CosmosDB statestore

Issue reference

Please reference the issue this PR will close: #1138

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 / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

@shivamkm07 shivamkm07 requested review from a team as code owners August 16, 2022 18:51
@codecov
Copy link

codecov bot commented Aug 16, 2022

Codecov Report

Merging #1972 (fcfcda7) into master (ebaa1b5) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1972      +/-   ##
==========================================
- Coverage   37.71%   37.67%   -0.04%     
==========================================
  Files         192      192              
  Lines       24016    24016              
==========================================
- Hits         9058     9049       -9     
- Misses      14189    14199      +10     
+ Partials      769      768       -1     
Impacted Files Coverage Δ
state/in-memory/in_memory.go 42.58% <0.00%> (-3.43%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

assert.Equal(t, expectedValue, string(item.Value))

// delete state
err = client.DeleteState(ctx, statestore, stateKey, setMeta)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also try to delete with wrong partition key and assert error?


## Test plan

### Basic Test using master key authentication
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this. It's already covered by conformance tests.


### Test Partition Keys
Ensure the following scenarios:
1. In case of invalid partition key, the access fails
Copy link
Member

Choose a reason for hiding this comment

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

Access? That sounds like it's about authorization.. but that's not so.

Please be specific that this is about a Get or Set operation.

1. Able to create and test connection.
2. Able to do set, fetch and delete.

### Test Partition Keys
Copy link
Member

Choose a reason for hiding this comment

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

Please add a test where you are connecting to a different collection -- a collection which does not use the expected /partitionKey partition key. We want to assert the failure for that case. This is a common problem.

To do so, please add a new collection with wrong partition name here:
https://github.com/dapr/components-contrib/blob/master/.github/infrastructure/conformance/azure/conf-test-azure-cosmosdb.bicep#L44

Then also make sure to update the following so that the second collection name gets exported to the KeyVault where we save these configurations
https://github.com/dapr/components-contrib/blob/master/.github/infrastructure/conformance/azure/conf-test-azure.bicep
https://github.com/dapr/components-contrib/blob/master/.github/infrastructure/conformance/azure/setup-azure-conf-test.sh

Lastly, set up a new component YAML talking to the collection with the bad partition name. And make sure to reference the new environment variable you are creating above.

Don't forget to inject that environment variable into the workflow run via the required-secrets section!

Copy link
Member

Choose a reason for hiding this comment

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

To provide more context -- what you have described is only impacting partition key values, but the Cosmos DB state store requires that the Cosmos DB collection was setup with a partition Key that is exactly called /partitionKey. It will be good to assert the failure when the collection has been set up wrong.

Copy link
Member

@berndverst berndverst left a comment

Choose a reason for hiding this comment

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

Please also add tests that check the handling of null data, empty string - especially with different content types specified.

#1974

@mukundansundar
Copy link
Contributor

Please also add tests that check the handling of null data, empty string - especially with different content types specified.

#1974

@berndverst should the nil, empty string test be part of conformance tests?

@DeepanshuA
Copy link
Contributor

@berndverst Please advise, does it make sense to include this test as well? :
#1138 (comment)

@berndverst
Copy link
Member

@berndverst Please advise, does it make sense to include this test as well? : #1138 (comment)

Ok to skip, I am going to add it here: #1992

Copy link
Member

@berndverst berndverst left a comment

Choose a reason for hiding this comment

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

@shivamkm07 please address review comments.

shivamkm07 and others added 3 commits September 8, 2022 14:48
Signed-off-by: shivam <shivamkm07@gmail.com>
Signed-off-by: shivam <shivamkm07@gmail.com>
Signed-off-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
Signed-off-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
Copy link
Member

@berndverst berndverst left a comment

Choose a reason for hiding this comment

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

We depend on this PR to upgrade the Track2 SDK. Therefore I will merge this now -- I fixed this up as necessary.

@dapr-bot dapr-bot merged commit 2b5650c into dapr:master Sep 8, 2022
berndverst added a commit to berndverst/components-contrib that referenced this pull request Sep 12, 2022
* Certification test for Azure CosmosDB

Signed-off-by: shivam <shivamkm07@gmail.com>

* Updating go.mod

Signed-off-by: shivam <shivamkm07@gmail.com>

* make motidy-all

Signed-off-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>

* CosmosDB Cert Test: Apply Dapr Runtime updates, Go version update etc

Signed-off-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>

Signed-off-by: shivam <shivamkm07@gmail.com>
Signed-off-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
Signed-off-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
shubham1172 pushed a commit to shubham1172/components-contrib that referenced this pull request Sep 20, 2022
* Certification test for Azure CosmosDB

Signed-off-by: shivam <shivamkm07@gmail.com>

* Updating go.mod

Signed-off-by: shivam <shivamkm07@gmail.com>

* make motidy-all

Signed-off-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>

* CosmosDB Cert Test: Apply Dapr Runtime updates, Go version update etc

Signed-off-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>

Signed-off-by: shivam <shivamkm07@gmail.com>
Signed-off-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
@berndverst berndverst added this to the v1.9 milestone Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CosmosDB state store as Stable
6 participants