WebAssembly / wasi-config

10 stars 8 forks source link

feat(error): add invalid-name error #19

Closed kate-goldenring closed 3 days ago

kate-goldenring commented 1 week ago

fixes https://github.com/WebAssembly/wasi-runtime-config/issues/18

What is the best way to bump/regenerate imports.md? It looks autogenerated.

badeend commented 1 week ago

It seems to me that invalid-name describes a reason why a key could not be inserted into the store.

But given that this interface only exposes the read-only side of the store, how can the consumer ever observe this? If any particular key does not exist in the store (regardless of why the key doesn't exist in the store), the get method is already documented to return ok(none)

Mossaka commented 1 week ago

I agree to what @badeend , and perhaps if we want to add invalid-name error, we need to change the semantics of get to not return ok(none) when the key is invalid. We will also need to elaborate on what a valid key really is. @devigned @thomastaylor312 any thoughts on this one?

itowlson commented 1 week ago

@badeend The motivation behind invalid-name is diagnostic. ok(none) tells the guest "I accept your request but sadly don't have that particular key." err(invalid-name) tells the guest "your expectations about what keys are legal don't chime with what this store supports, things may not work as you expect."

That said: there is not really much the guest can do in this situation, and it's arguable whether "go ahead with your best defaults, or fail with a not found if you have no default" or "report what is surely an admin/deployment error to the guest itself" is the better option. The error is certainly better reported by logging than by returning it to a user, and that could be done by the host. But we wouldn't skip reporting a transient upstream error to the guest, even if the guest can't do much with it: why skip reporting a configuration error?

Anyway, as @kate-goldenring says on #18, this is definitely not a blocker, and I totally get the case for sticking with ok(none) and handling the misconfiguration out of band. I'm fine with whatever decision is made - just offering this as context.

thomastaylor312 commented 1 week ago

I can see both sides of the argument here. On the one hand it is nice to know why a failure happened, especially if you're bringing in an external component that you might not know what config keys they could end up requesting. But also I think it is completely correct that ok(none) is also accurate as @badeend pointed out.

Because versions are free and we can always add more later, would it make it absolutely terrible for your use case @kate-goldenring if we didn't put it in now, and if it becomes a problem, we add a new error case later?

kate-goldenring commented 3 days ago

@thomastaylor312 I'll go ahead and close this. We can bring this into a new version if others want this more descriptive error resolution