Allow configuration to be passed in using TEDGE_ environment variables #1790
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jarhodes314
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Robot Results
Passed Tests
|
jarhodes314
force-pushed
the
feat/config-env-vars
branch
from
3d49797
to
eb387b7
Compare
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
jarhodes314
force-pushed
the
feat/config-env-vars
branch
from
eb387b7
to
2c5dab5
Compare
jarhodes314
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
didier-wenzek
approved these changes
Mar 3, 2023
jarhodes314
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
| config_repository: context.config_repository, | ||
| config_repository: context.config_repository.skip_environment_variables(), |
There was a problem hiding this comment.
Why does the tedge connect require write access to the config?
This seems to an issue:
- I don't see what can be the point here.
- Using
tegde connectin combination with $ENV setting is one of the main point of this PR.
There was a problem hiding this comment.
tedge connect updates tedge.toml with updated MQTT settings. It obviously also updates the mosquitto config with the relevant settings, but making that work in the context of thin-edge running in a container requires some sort of persistence for /etc/tedge anyway, so I'm not entirely sure what we gain by avoiding using /etc/tedge/tedge.toml in that case? @reubenmiller can you provide some insight as to how you're expecting this feature to be used?
There was a problem hiding this comment.
I'm wondering whether instead of opt-out of environment variables entirely when storing values, we should replace the store method with an update_toml method that takes an argument impl FnOnce(Config) -> Config. The repository calls that with only the TOML config, and saves whatever is returned back to the TOML file. This allows us to better scope the updates to the TOML config in cases like this where we may want to refer to environment variables but also update the persistent configuration.
There was a problem hiding this comment.
tedge connect updates tedge.toml with updated MQTT settings.
This comes as a surprise to me. I really don't understand what's the point.
There was a problem hiding this comment.
tedge connectupdates tedge.toml with updated MQTT settings. It obviously also updates the mosquitto config with the relevant settings, but making that work in the context of thin-edge running in a container requires some sort of persistence for/etc/tedgeanyway, so I'm not entirely sure what we gain by avoiding using/etc/tedge/tedge.tomlin that case? @reubenmiller can you provide some insight as to how you're expecting this feature to be used?
Idea is to create multiple containers where each container runs one component of thin-edge, e.g. one container for tedge-agent, one for tedge-mapper-c8y, one for mosquitto. Each container will be configured to use an external mqtt broker by setting the mqtt client settings to point to the mosquitto broker (via environment variables). The actual values inside the tedge.toml don't need to store the values set in the environment variables.
The tedge connect command will only be used to setup the mosquitto broker bridge settings (or in some cases it will not be used at all, as technically the user can create the bridge settings themselves).
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
jarhodes314
force-pushed
the
feat/config-env-vars
branch
from
a2986fa
to
128a77a
Compare
jarhodes314
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
didier-wenzek
approved these changes
Mar 7, 2023
| fn store(&self, config: &TEdgeConfig) -> Result<(), TEdgeConfigError> { | ||
| let toml = toml::to_string_pretty(&config.data)?; | ||
|
|
||
| // Create `$HOME/.tedge` or `/etc/tedge` directory in case it does not exist yet |
There was a problem hiding this comment.
The idea to use $HOME/.tedge has been fully deprecated. Now, the default tedge config directory is /etc/tedge and can be provided on the command line using the --tedge-config option.
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
jarhodes314
force-pushed
the
feat/config-env-vars
branch
from
b598782
to
9e72593
Compare
jarhodes314
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Proposed changes
Add support for reading
TEDGE_prefixed environment variables, as detailed in #1783.e.g.
Types of changes
Paste Link to the issue
#1783
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments
The main change, using figment instead of deserialising directly from the TOML file is quite simple, but generating error messages which contain the relevant environment variable names requires quite a bit of effort. When using things like this though, I think it's really helpful to understand where an incorrect configuration came from, so I think this is ultimately worth the effort. I've also refactored the warnings to use
serde_ignored(again, in an annoyingly complex way), but this does mean we no longer needtedge-deriveor thedisplay_unknown_formacro which had to be called manually for every field.