Add kv_namespaces in metadata #215

xtuc · 2019-06-05T09:52:07Z

Fixes #92

src/commands/build/mod.rs

xtuc · 2019-06-10T13:59:50Z

I'm not super happy but It kinda works, you need to specifiy the namespace_id in your wrangler.toml.

# ...

[[kv-namespaces]]
local_binding = "local_binding"
name = "name"
namespace_id = "abc"

kristianfreeman · 2019-06-14T15:08:53Z

Re: the namespace_id comment - I agree that having to specify it inside of wrangler.toml isn't great, but if users are creating the bindings from inside of a Wrangler-enabled project, can we write it to wrangler.toml for them on initial creation?

There may be situations where they'll need to go look up the namespace manually, but I imagine the average workflow will take place (hopefully!) in Wrangler, so we should know the namespace_id of a KV store since we're creating it for them during the publishing process.

(BTW - I am super motivated to help make this work land; it's a HUGE pain right now to have to keep the editor UI open right now to re-bind a KV namespace after each publish)

steveklabnik · 2019-06-20T15:03:48Z

What is currently holding this PR back from landing?

xtuc · 2019-06-20T15:25:48Z

@steveklabnik it would require a couple of rebases but I'm happy to see this landing. If you need it I can continue to work on it.

steveklabnik · 2019-06-20T16:02:14Z

It'd be nice to get it landed so that we can continue the work on KV support overall; if you've got the time that would be great.

ashleymichal · 2019-06-21T14:31:59Z

I think what I'm missing to be able to judge this PR is the README section on how to configure your namespaces in the wrangler.toml; @xtuc touched on it in his comment but what would my wrangler.toml look like if I had two or more namespaces exposed to my worker?

steveklabnik · 2019-06-21T16:55:00Z

@ashleymichal it currently looks like this:

kv-namespaces = ["STATIC_CONTENT"]

if you had two

kv-namespaces = ["STATIC_CONTENT", "FOOBAR"]

ashleymichal · 2019-06-21T18:24:13Z

@steveklabnik based on the earlier comment from @xtuc here, that does not appear to be the case in the context of this PR; the user is required to specify not only the name of the binding as it should appear in the Worker, but the "title" and id of the namespace, in order to both create the namespace (if it does not already exist), and specify the binding in the metadata.

The question still stands, as once we provide any interface to users, it becomes difficult to change. In order to land this, we need to care about a few things:

Providing enough information to allow Wrangler to provision a KV namespace on behalf of a user OR requiring the user to provision the namespace before integrating with their Worker project.
Providing enough information for Wrangler to generate a KV Binding entry for the metadata part of the upload form. This can either be provided by the user manually (asking them to enter their namespace id manually as shown in @xtuc 's comment), or it can be retrieved by Wrangler behind the scenes with a query to the KV API based on the namespace title.
Providing configuration of the relationship between the binding variable name and the namespace title. This can be done as it stands currently (by assuming they are one and the same and requiring they adhere to JavaScript variable name standards), or it can be done more manually as specified in @xtuc 's comment.

I think 1. is well handled here and lays a decent enough foundation for later improvements and features.

In the case of 2 and 3, I think it would be short-sighted to go with the seemingly "simpler" approach of requiring the user to only provide the namespace title. It is much easier in the long term to make those three connections explicitly and provide "sane defaults" later than to do the reverse. We should therefore take the time to consider what the entries should look like in the wrangler.toml now.

ashleygwilliams · 2019-06-21T19:04:29Z

config wise- re multiple kv name spaces i'd probably do something like this... keying on "title".. i could also see us keying on local binding. i personally find the existence of both to be redundant (open to hear the motivation and have my mind changed, of course).

# Cargo.toml

[kv-namespaces]
title1 = { local_binding: "thing", id : "aksdhjfkahg;a"}

# or,  its equivalent

[kv-namespaces.title1]
local_binding = "thing"
id = "aksdhjfkahg;a"

collapsing the "local_binding" and "title" (assuming they obey the property with the most constraints constraints...) we could do this instead:

[kv-namespaces]
title1 = "aksdhjfkahg;a"
title2 = "<id string>"

this is preferrable IMO because it now looks like a list of dependencies! which is nice.

ashleygwilliams · 2019-06-21T19:04:56Z

it should also be noted this is only for the webpack project type, and we have 3 types... choosing to release it for a single type should be a deliberate decision.

steveklabnik · 2019-06-21T21:57:52Z

Ah, see when I implemented it, it used the title of the namespace and made a binding of the same name; we decided not to support the case where the two differ.

…

On Jun 21, 2019, at 2:04 PM, ashley williams ***@***.***> wrote: it should also be noted this is only for the webpack project type which is a deliberate decision we should make — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

xtuc · 2019-06-25T10:15:44Z

Maybe we could iterate on this PR, to get something useful I would:

remove the auto creation of namespaces (because we don't store the id). The user/we should do it from the UI.
let the namespace_id, name and binding name be in the wrangler.toml

This will unlock our deployement and provide a base used for future iterations. We shouldn't document it for now tho. What do you think?

cwndrws · 2019-06-27T14:14:53Z

I agree that the KV config should be in wrangler.toml. I like the idea of requiring all three values: (title, id, and binding) like @ashleymichal suggested. I think requiring title be a valid JS symbol would bite us eventually.

src/commands/build/wranglerjs/metadata.rs

xtuc · 2019-07-01T09:56:34Z

I made an update, mainly based on #215 (comment).

@cwndrws comments:

I agree that the KV config should be in wrangler.toml. I like the idea of requiring all three values: (title, id, and binding) like @ashleymichal suggested.

The kv-namespace configuration is only possible in wrangler.toml; I removed the auto-creation feature (for now).

The config looks like the following:

[[kv-namespaces]]
name = "KV_NAMESPACE_A"
id = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"

I'm not sure we should ask the user for the namespace name because we won't use it, the API doesn't support it. It would be more confusing if these names get out-of-sync and it would still work, until we add a check which would be a breaking change.

I think requiring title be a valid JS symbol would bite us eventually.

I like that idea actually, I updated #90 and we can discuss there.

Merge #285 first.

cwndrws · 2019-07-01T14:54:07Z

@xtuc

I'm not sure we should ask the user for the namespace name because we won't use it, the API doesn't support it. It would be more confusing if these names get out-of-sync and it would still work, until we add a check which would be a breaking change.

I agree with not using the namespace name.

Your example config:

[[kv-namespaces]]
name = "KV_NAMESPACE_A"
id = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"

doesn't contain a binding symbol. Should it be:

[[kv-namespaces]]
name = "KV_NAMESPACE_A"
binding = "KV_NAMESPACE_A"
id = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"

Or something similar?

xtuc · 2019-07-01T16:03:55Z

@cwndrws the name is the binding, I'm happy to rename it if it's confusing to you.

ashleymichal · 2019-07-01T22:31:10Z

upon further reflection, i actually do think that long term this is the right way to specify these namespaces. the wrangler.toml for a workers project should regard kv namespaces as an external dependency, and the configuration of that dependency is a separate issue.

@cwndrws the name is the binding, I'm happy to rename it if it's confusing to you.

i too am struggling with the correct key for name but unless we can find something sufficiently descriptive we may need to rely on documentation. binding may be fine; varName is also maybe a thing?

Allow the user to declare kv-namespaces in the configuration and writes the binding to the worker metadata. Adds a new binding type `kv_namespace`. Extends the Wrangler configuration to add: - `kv-namespaces`: Bind kv namespaces to your worker; an array of: - `binding`: name that will be used to bind the kv-namespace to your script, in JavaScript. - `id`: Identifer of the namespace. For example: ```toml [[kv-namespaces]] binding = "KV_NAMESPACE_A" id = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" [[kv-namespaces]] binding = "KV_NAMESPACE_B" id = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" ``` The documentation (README.md) has been updated with this configuration change. Fixes #92

xtuc · 2019-07-02T09:32:30Z

I noticed that when the namespace id is wrong, the publishing API returns a unclear error (generic 400, workers.api.error.internal_server); it would be great to change it.

@ashleymichal I did the updated.

xtuc · 2019-07-03T13:20:53Z

Merging into feat-better-kv

juancampuzano · 2019-07-15T16:49:33Z

Is it possible to make kv-namespaces in a production project that uses wrangler or is it a feature in development?

Currently I do not see the documentation of this in the master.

ashleygwilliams added the work in progress label Jun 5, 2019

xtuc mentioned this pull request Jun 5, 2019

wrangler deploy un-does KV namespace bindings #92

Closed

steveklabnik reviewed Jun 7, 2019

View reviewed changes

src/commands/build/mod.rs Outdated Show resolved Hide resolved

xtuc requested a review from steveklabnik June 10, 2019 13:58

xtuc mentioned this pull request Jun 15, 2019

Document kv-namespaces in README #203

Closed

ashleygwilliams added the changelog - feature label Jun 20, 2019

ashleygwilliams added this to the 1.1.0 milestone Jun 20, 2019

ashleygwilliams requested review from ashleygwilliams and ashleymichal June 20, 2019 23:19

ashleygwilliams changed the title ~~Add kv_namespaces in metadata~~ [WIP] Add kv_namespaces in metadata Jun 20, 2019

ashleygwilliams removed this from the 1.1.0 milestone Jun 24, 2019

ashleymichal mentioned this pull request Jun 25, 2019

Use Metadata struct #217

Closed

xtuc mentioned this pull request Jun 27, 2019

Refactor Bindings to support all binding types #273

Closed

cwndrws reviewed Jun 27, 2019

View reviewed changes

src/commands/build/wranglerjs/metadata.rs Outdated Show resolved Hide resolved

xtuc force-pushed the feat-add-kv_namespaces-bindings branch from 55042eb to 0c3fef2 Compare July 1, 2019 08:19

xtuc marked this pull request as ready for review July 1, 2019 08:19

xtuc force-pushed the feat-add-kv_namespaces-bindings branch from 0c3fef2 to 34a6b22 Compare July 1, 2019 08:25

ashleygwilliams assigned ashleymichal Jul 1, 2019

This comment has been minimized.

Sign in to view

xtuc force-pushed the feat-add-kv_namespaces-bindings branch 4 times, most recently from 75a189b to dc90d8f Compare July 2, 2019 09:30

xtuc changed the title ~~[WIP] Add kv_namespaces in metadata~~ Add kv_namespaces in metadata Jul 2, 2019

xtuc force-pushed the feat-add-kv_namespaces-bindings branch from dc90d8f to d7fbd78 Compare July 2, 2019 09:30

xtuc removed the status - work in progress label Jul 2, 2019

This was referenced Jul 2, 2019

add some tests #292

Merged

RFC: expand usage of the Bundle #293

Closed

xtuc changed the base branch from master to feat-better-kv July 2, 2019 17:09

ashleymichal approved these changes Jul 3, 2019

View reviewed changes

xtuc merged commit 35f7d0e into feat-better-kv Jul 3, 2019

delete-merged-branch bot deleted the feat-add-kv_namespaces-bindings branch July 3, 2019 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add kv_namespaces in metadata #215

Add kv_namespaces in metadata #215

xtuc commented Jun 5, 2019

xtuc commented Jun 10, 2019 •

edited

Loading

kristianfreeman commented Jun 14, 2019 •

edited

Loading

steveklabnik commented Jun 20, 2019

xtuc commented Jun 20, 2019

steveklabnik commented Jun 20, 2019

ashleymichal commented Jun 21, 2019

steveklabnik commented Jun 21, 2019

ashleymichal commented Jun 21, 2019

ashleygwilliams commented Jun 21, 2019 •

edited

Loading

ashleygwilliams commented Jun 21, 2019 •

edited

Loading

steveklabnik commented Jun 21, 2019 via email

xtuc commented Jun 25, 2019

cwndrws commented Jun 27, 2019

xtuc commented Jul 1, 2019 •

edited

Loading

cwndrws commented Jul 1, 2019

xtuc commented Jul 1, 2019

This comment has been minimized.

ashleymichal commented Jul 1, 2019 •

edited

Loading

xtuc commented Jul 2, 2019

xtuc commented Jul 3, 2019

juancampuzano commented Jul 15, 2019 •

edited

Loading

Add kv_namespaces in metadata #215

Add kv_namespaces in metadata #215

Conversation

xtuc commented Jun 5, 2019

xtuc commented Jun 10, 2019 • edited Loading

kristianfreeman commented Jun 14, 2019 • edited Loading

steveklabnik commented Jun 20, 2019

xtuc commented Jun 20, 2019

steveklabnik commented Jun 20, 2019

ashleymichal commented Jun 21, 2019

steveklabnik commented Jun 21, 2019

ashleymichal commented Jun 21, 2019

ashleygwilliams commented Jun 21, 2019 • edited Loading

ashleygwilliams commented Jun 21, 2019 • edited Loading

steveklabnik commented Jun 21, 2019 via email

xtuc commented Jun 25, 2019

cwndrws commented Jun 27, 2019

xtuc commented Jul 1, 2019 • edited Loading

cwndrws commented Jul 1, 2019

xtuc commented Jul 1, 2019

This comment has been minimized.

ashleymichal commented Jul 1, 2019 • edited Loading

xtuc commented Jul 2, 2019

xtuc commented Jul 3, 2019

juancampuzano commented Jul 15, 2019 • edited Loading

xtuc commented Jun 10, 2019 •

edited

Loading

kristianfreeman commented Jun 14, 2019 •

edited

Loading

ashleygwilliams commented Jun 21, 2019 •

edited

Loading

ashleygwilliams commented Jun 21, 2019 •

edited

Loading

xtuc commented Jul 1, 2019 •

edited

Loading

ashleymichal commented Jul 1, 2019 •

edited

Loading

juancampuzano commented Jul 15, 2019 •

edited

Loading