-
Notifications
You must be signed in to change notification settings - Fork 19
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
Make container environment variables accessible as application variables #149
Make container environment variables accessible as application variables #149
Conversation
2a32c71
to
ce35a35
Compare
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.
Added some comments, this overall LGTM!
02ff7fb
to
5d8868a
Compare
I updated the implementation to check the |
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.
Quick comment; otherwise looks good to me but defer to previous/requested reviewers.
@@ -43,6 +43,9 @@ const OCI_LAYER_MEDIA_TYPE_WASM: &str = "application/vnd.wasm.content.layer.v1+w | |||
/// Expected location of the Spin manifest when loading from a file rather than | |||
/// an OCI image | |||
const SPIN_MANIFEST_FILE_PATH: &str = "/spin.toml"; | |||
/// Known prefix for the Spin application variables environment variable | |||
/// provider: https://github.com/fermyon/spin/blob/436ad589237c02f7aa4693e984132808fd80b863/crates/variables/src/provider/env.rs#L9 | |||
const SPIN_APPLICATION_VARIABLE_PREFIX: &str = "SPIN_VARIABLE"; |
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.
Follow-up perhaps: Update https://github.com/fermyon/spin/blob/436ad589237c02f7aa4693e984132808fd80b863/crates/variables/src/provider/env.rs#L9 to pub
and use directly here?
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 not sure it is worth bringing in the variables crate for this constant (if we did make it pub). Maybe with the spin factors work, this will be more directly configured by the shim
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.
LGTM, added a few suggestions
5d8868a
to
4554a80
Compare
- Sets any non-execution environment variables in the Spin app unless a disable env var is set Signed-off-by: Kate Goldenring <[email protected]>
…s application variables Signed-off-by: Kate Goldenring <[email protected]>
Signed-off-by: Kate Goldenring <[email protected]>
Signed-off-by: Kate Goldenring <[email protected]>
4554a80
to
40fa6df
Compare
Just rebased. @Mossaka @jsturtevant @devigned @radu-matei this is ready to review. |
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.
LGTM!
Prefixes all container env vars with
SPIN_VARIABLE
. This means that Spin components can get access to these variables if specifically configured in the Spin manifest to have access. For example, say a a component wants access to theKUBERNETES_SERVICE_ADDRESS
container environment variable, it would configure this in the Spin.toml as follows:Then, it can be accessed in the Spin app using the variables SDK:
Should affect/reflect the "Configuring Runtime Options" SKIP spinframework/skips#4