-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat(galaxy|access): "appdata" 128kb storage api #2763
feat(galaxy|access): "appdata" 128kb storage api #2763
Conversation
case 'enable': | ||
await coreClient.starbase.setExternalDataPackage.mutate({ | ||
clientId, | ||
packageType: ExternalDataPackageType.BASE, |
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.
Let's name this STARTER, to future-proof it a little.
const node = initAuthorizationNodeByName(urn, ctx.env.Authorization) | ||
|
||
const externalStorageReads = await ctx.env.UsageKV.get<number>( | ||
`${clientId}:external-storage:read` |
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.
Externalize this into function that gives you the usage key.
@@ -0,0 +1,7 @@ | |||
query getExternalData { |
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.
Let's rename references of this to *ExternalAppData
, here and everywhere else in the PR, to differentiate it from future external data features.
const node = initAuthorizationNodeByName(urn, ctx.env.Authorization) | ||
|
||
const externalStorageWrites = await ctx.env.UsageKV.get<number>( | ||
`${clientId}:external-storage:write` |
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.
Same as previous comment on externalizing this into a function.
}) | ||
} | ||
|
||
await node.storage.put('externalData', payload) |
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 and the await
below should be run concurrently
}) | ||
} | ||
|
||
const externalData = await node.storage.get('externalData') |
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 and promise below should run concurrently.
@@ -6,6 +6,10 @@ logpush = true | |||
node_compat = true | |||
workers_dev = false | |||
|
|||
kv_namespaces = [ | |||
{ binding = "UsageKV", id = "UsageKV", preview_id = "UsageKV" }, |
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 the env-specific wrangler files would also need to be updated.
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.
Yes, I'll update them once we reach the final design.
ExternalDataPackageDefinition | undefined | ||
> { | ||
return this.state.storage.get<ExternalDataPackageDefinition>( | ||
'externalDataPackage' |
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 key name and the one below for the next function don't match.
}, | ||
] | ||
|
||
export default packages.reduce((acc, curr) => { |
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.
Shouldn't this be set statically in code (map or object) as opposed to being calculated at runtime?
01575c8
to
ad59884
Compare
c7c8bea
to
939a499
Compare
@Cosmin-Parvulescu copy for early access panel and service description inside storage page: Remove API URL from currently-hidden app data storage page itself. |
0420ed3
to
384100e
Compare
384100e
to
928b44b
Compare
...generateTraceContextHeaders(traceSpan), | ||
}) | ||
|
||
return coreClient.authorization.getExternalAppData.query({ |
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.
In the case of external storage not being defined, the error exposed through Galaxy is 400 Bad request, yet the TRPC one comes through correctly as external storage not enabled
payload, | ||
}) | ||
} catch (ex) { | ||
console.error(ex) |
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.
Related to comment above, when external storage is not enabled, this returns false as opposed to throwing the exception being thrown for the getExternalAppData
query.
This is good to go; will be approved and merged once #2743 is in. |
storage section
composition
update type definition
composition
router for enabling/disabling features
tracking in getExternalData and setExternalData methods
mismatched key & validator file
external app data package
getExternalAppData and setExternalAppData methods
getExternalAppData and setExternalAppData
getExternalAppData and setExternalAppData
…in authorization resolver
b055bae
to
8555c6f
Compare
Description
Related Issues
Testing
Console
<current_url>/ostrich
Galaxy
{ /"foo/":/"bar/"}
Profile
profile.tsx
routeChecklist