Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement wasi-config #2869
Implement wasi-config #2869
Changes from 1 commit
d7fb38d
942de93
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 am slightly worried about performance - at some point we might want to see if adding some sort of caching mechanism to providers is worth it.
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.
We should probably push this down into the provider as e.g. (untested):
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 don't think any of our current providers can optimise this beyond running the futures concurrently - all appear to support retrieving only one secret at once - so I propose we defer this.
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.
@itowlson instead of adding this function, could we use lann's suggestion from
get_all
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 we'll want to consider whether we want to make this configurable for users of the factor - i.e., do we want to force all runtimes that use the
VariablesFactor
to link all three interfaces? We could instead make this linking conditional based on some configuration provided when the factor is constructed.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.
Crate feature flag?
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.
Seems like the feature is light-weight enough that we could just do a dynamic check instead of a compile time feature flag, but I don't feel strongly either way.
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.
We'll need to make a decision on this (and a similar consideration for wasi-keyvalue) fairly soon if we hope to land this in Spin 3.
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.
My suggestion is just an API concern for those using this factor as library (in alternate runtimes). Whether Spin CLI turns this feature on, puts it behind a feature flag, or something else, is unrelated to whether the factor allows embedder to turn on wasi config or not.
That all being said, I'm highly in favor of allowing embedders to control what gets linked. I have less strong feelings about how that gets accomplished, but I think I prefer a dynamic check (i.e., a bool, enum, or maybe even bitflags that the embedder passes to the factor to indicate which interfaces should be linked in).
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.
It sounds like you're expressing a general principle about allowing embedders to control which interfaces get linked, rather than anything specific to wasi-config, or am I misunderstanding?
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
We should consider adding a conformance test for this. The runtime tests here are meant to be Spin CLI specific but this test seems like it should be applied to all Spin compliant runtimes.
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 can't do that as part of this PR but I've raised fermyon/conformance-tests#40 to remind us to do so once this merges.