-
Notifications
You must be signed in to change notification settings - Fork 500
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
state: add RavenDB support #3702
base: main
Are you sure you want to change the base?
Conversation
Hey @nmalocic thank you for your contribution! |
@nmalocic could you use a more meaningful title? |
31aa273
to
5064086
Compare
@elena-kolevska Commit is signed and now I see pending conformance tests, is there anything to fix before they are runned? @daixiang0 Is title better now, should I also update description? |
Please remove issue number in the title, format as "state: add RavenDB support". Also update description. |
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.
Thanks Patrick!
There's a lint error preventing cert tests to run.
Also, I didn't see the ravendb conformance test in the executed actions, it needs to be added in there (I think in the .github/scripts/test-info.mjs
file). Check this pr for reference: #3588
…or cleaner history Signed-off-by: Nemanja Malocic <[email protected]>
Signed-off-by: Nemanja Malocic <[email protected]>
Signed-off-by: Nemanja Malocic <[email protected]>
Signed-off-by: Nemanja Malocic <[email protected]>
Signed-off-by: Nemanja Malocic <[email protected]>
161c4e4
to
53fef16
Compare
Signed-off-by: Nemanja Malocic <[email protected]>
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.
Overall looks good, just a few small things and also check the lint failures pls.
Great work!
ports: | ||
- "8080:8080" | ||
environment: | ||
- RAVEN_LICENSE={"Id":"1bcf6b3f-cd52-488c-9aa2-c982d6e57c78","Name":"RavenDB","Keys":["CtmBDhwRJAJIDOHv+Pev+B8o4","A5l3K4n1z2FbtgHsKPl1QkQNx","LPqr9nkPEq5MUCHQZ0XkkP5BY","Czc9DkF33Y8mNFemJZl8tkGm6","DY2Q6YyYtEt/jQayDOUVUtUlY","FC4inkD4zkb3Ii2WEbyv5RRew","jzoKxeOVYXQsToAMe6xG9ABYE","DNy4wBSYoSQMqKywtLi8wJzEy","MzQVFjc4OTo7PD0+nwIfIJ8CI","CCfAiEgnwIjIJ8CJCCfAiUgnw","ImIJ8CJyCfAiggnwIpIJ8CKiC","fAisgnwIsIJ8CLSCfAi4gnwIv","IJ8CMCCfAzZAAZ8CQiBDJEQJY","htbnwRBYBhb"]} |
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 need to put this in a secret and get a license that never expires
apiVersion: dapr.io/v1alpha1 | ||
kind: Configuration | ||
metadata: | ||
name: keyvaultconfig | ||
spec: | ||
features: |
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 is this needed?
err = client.SaveState(ctx, stateStoreName, certificationTestPrefix+"key2", []byte("ravenCert2"), nil) | ||
require.NoError(t, err) |
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 think we should either get this and verify the value, like we do for the first key, or remove it.
"ttlInSeconds": "-1", | ||
})) | ||
require.NoError(t, client.SaveState(ctx, stateStoreName, certificationTestPrefix+"ttl3", []byte("revendbCert3"), map[string]string{ | ||
"ttlInSeconds": "3", |
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 suggest we make this a higher value cause CI runners are slow and sometimes the key can expire before we get it.
assert.Contains(t, item.Metadata, "ttlExpireTime") | ||
expireTime, err := time.Parse(time.RFC3339, item.Metadata["ttlExpireTime"]) | ||
require.NoError(t, err) | ||
assert.InDelta(t, time.Now().Add(time.Second*3).Unix(), expireTime.Unix(), 2) |
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 safer to set a now
var just before you save the key, for the same reason as above - slow CI runners.
}, | ||
{ | ||
Type: daprClient.StateOperationTypeUpsert, | ||
Item: &daprClient.SetStateItem{ |
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.
Would be nice if we also added a delete operation here
Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Nemanja Malocic <[email protected]>
Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Nemanja Malocic <[email protected]>
Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Nemanja Malocic <[email protected]>
Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Nemanja Malocic <[email protected]>
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.
This looks good to me and can be merged as soon as the licence key is removed from code and set as an env variable.
Added files related to RavenDB implementation from old branch for cleaner history
Description
Added support for RavenDB as state store.
Support for Set, Get, Delete and Multi interfaces.
Added conformance and certification tests
Issue reference
Please reference the issue this PR will close: #3318
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: