Config serialization should take into account environment variables #4865
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
ypatia
force-pushed
the
yt/sc-44928/fix_config_serialization
branch
from
e4ad636 to
f52ab26
Compare
ypatia
force-pushed
the
yt/sc-44928/fix_config_serialization
branch
from
f52ab26 to
4305980
Compare
Comment on lines
+714
to
+717
| /** Get all config params taking into account environment variables */ | ||
| const std::map<std::string, std::string> get_all_params_from_config_or_env() | ||
| const; | ||
|
|
There was a problem hiding this comment.
Maybe a better way would be to populate the config options with the environment variables at the time the Config gets created, but it might introduce complications when unsetting values.
KiterLuc
approved these changes
Apr 12, 2024
dudoslav
pushed a commit
that referenced
this pull request
Apr 17, 2024
…4865) sc-44928 We were getting failures while testing Query v3 , and specifically when setting up the REST-CI environment for it, because the `rest.use_refactored_array_open_and_query_submit` config we were setting was not getting propagated to the REST server. After debugging it seems that Config serialization code was calling `param_values()` method to get the config variables to set in Cap'n'proto, which is not taking into account environment variables. This PR also moves `param_values()` to private to prevent other classes from similar mistakes. `ConfiIter` is the only class that still uses it and I only saw that class being used in `S3Parameters::load_headers`. It's worth investigating further if that's a problem or not. --- TYPE: BUG DESC: Config serialization should take into account environment variables
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.
sc-44928
We were getting failures while testing Query v3 , and specifically when setting up the REST-CI environment for it, because the
rest.use_refactored_array_open_and_query_submitconfig we were setting was not getting propagated to the REST server.After debugging it seems that Config serialization code was calling
param_values()method to get the config variables to set in Cap'n'proto, which is not taking into account environment variables.This PR also moves
param_values()to private to prevent other classes from similar mistakes.ConfiIteris the only class that still uses it and I only saw that class being used inS3Parameters::load_headers. It's worth investigating further if that's a problem or not.TYPE: BUG
DESC: Config serialization should take into account environment variables