Add support for text blob bindings #1543
Conversation
correct. there's another ticket for that: #896 |
src/settings/toml/manifest.rs
Outdated
| @@ -43,6 +43,7 @@ pub struct Manifest { | |||
| pub kv_namespaces: Option<Vec<ConfigKvNamespace>>, | |||
| pub env: Option<HashMap<String, Environment>>, | |||
| pub vars: Option<HashMap<String, String>>, | |||
| pub blobs: Option<HashMap<String, String>>, | |||
There was a problem hiding this comment.
you should be able to use a <HashMap<String, PathBuf>> explicitly
There was a problem hiding this comment.
I'd have used PathBuf, but webpack_config above uses String so I used that, but I can change it if you'd like.
src/settings/toml/manifest.rs
Outdated
| @@ -239,6 +241,9 @@ impl Manifest { | |||
|
|
|||
| // don't inherit vars | |||
| target.vars = environment.vars.clone(); | |||
|
|
|||
| // don't inherit blobs | |||
| target.blobs = environment.blobs.clone(); | |||
There was a problem hiding this comment.
i don't remember why we chose not to inherit vars... 🤔
There was a problem hiding this comment.
i think these should likely be inherited; cc @rita3ko any thoughts?
There was a problem hiding this comment.
these should be inherited i think.
There was a problem hiding this comment.
My only reason for not inheriting these, is it feels weird to do something different than for vars.
There was a problem hiding this comment.
@ashleymichal @rita3ko Do y'all have a final opinion here as to why these should be inherited when vars are not? Y'all have more context than I do about what should/should not get inherited
EDIT:yeah, I see why these should be inherited, since they're more likely to represent something that is inherent to the worker itself, rather than it's environment. I changed it to inherit, can always drop the commit if y'all change your minds.
|
@rita3ko : "Let's call 'em text blobs specifically, and just support those" |
|
lgtm once the name change is complete. |
ashleymichal
dismissed
their stale review
would like comments above to be addressed
|
@xortive are you in a position to address the above comments or should someone pick this up and carry it over the finish line? |
Sorry I've been unresponsive with this PR, I've got some free time today so I'm going to get this out the door |
xortive
force-pushed
the
malonso/483-text-binding
branch
from
fdb8166
to
994b1e1
Compare
|
name change done, going to resolve conflicts now |
|
Alright, looks like this should be ready to go |
closes #483
I need to write docs, and confirm I got the env inheritance stuff right (I copied how it works for vars, need to ensure that is correct.)
But my baby test passes :)
I probably need to ensure size limits are still taken into account here as well.
changelog/docs:
Wrangler now supports text blobs! text blobs are values to use in your workers, but read from a file instead of a string in your TOML.
Usage:
text_blobs = { FOO = "path/to/foo.txt", BAR = "path/to/bar.txt" }