KV store generation integer follow up (#1029) #1108
Conversation
| @@ -11,6 +11,9 @@ const { DICTIONARY_NAME, CONFIG_STORE_NAME, KV_STORE_NAME, SECRET_STORE_NAME } = | |||
| getEnv(serviceName); | |||
|
|
|||
| function existingStoreId(stores, existingName) { | |||
There was a problem hiding this comment.
in setting up my fastly secret-store, it got into a state where it was null and throw an exception. Not sure if this is necessary or just something I encountered while trying to setup the integration tests.
There was a problem hiding this comment.
Strange, it would be interesting to dig into the caller of this function, to see what that is happening and potentially resolve the issue at the root.
| if (!JS_GetProperty(cx, opts_val, "gen", &gen_val)) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
the generation match integer comes across as a uint64_t, however I'm not converting it to uint64. I noticed some functions in spidermonkey that had a comment of it being non-standard, could/should I be using those?
There was a problem hiding this comment.
Adding the support for BigInt can be useful for 64 bit integers effectively supporting gen: 1000n as well as gen: 1000 say. The BigInt functions are exactly what you want to use and there should be existing examples in the codebase for this as well.
There was a problem hiding this comment.
I couldn't find a good example of converting a RootedBigInt and converting that to a uint64_t, so I left it as a number
| if (!JS_GetProperty(cx, opts_val, "gen", &gen_val)) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Adding the support for BigInt can be useful for 64 bit integers effectively supporting gen: 1000n as well as gen: 1000 say. The BigInt functions are exactly what you want to use and there should be existing examples in the codebase for this as well.
| @@ -11,6 +11,9 @@ const { DICTIONARY_NAME, CONFIG_STORE_NAME, KV_STORE_NAME, SECRET_STORE_NAME } = | |||
| getEnv(serviceName); | |||
|
|
|||
| function existingStoreId(stores, existingName) { | |||
There was a problem hiding this comment.
Strange, it would be interesting to dig into the caller of this function, to see what that is happening and potentially resolve the issue at the root.
Thanks I updated the related documentation, I'd appreciate your input on the wording. I'll look to add more error cases. |
|
You can run For the other test failures, this is actually a CI issue that we don't clean up the secret stores properly. I've posted #1110. Otherwise looks like this is good to land to me! |
follow up to #1029