feat(jans-cedarling): Load bootstrap properties from environment variables #10692

olehbozhok · 2025-01-18T00:13:45Z

Prepare

Read PR guidelines
Read license information

Description

Implemented load BootstrapConfig from environment variables

Target issue

link

closes #10648

Implementation Details

Implemented load BootstrapConfig from environment variables
updated python bindings
updated flask-sidecar

Test and Document the changes

Static code analysis has been run locally and issues have been fixed
Relevant unit and integration tests have been added/updated
Relevant documentation has been updated if any (i.e. user guides, installation and configuration guides, technical design docs etc)

Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with docs: to indicate documentation changes or if the below checklist is not selected.

I confirm that there is no impact on the docs due to the code changes in this PR.

…alize` that allows to deserialize struct from string Signed-off-by: Oleh Bohzok <[email protected]>

Signed-off-by: Oleh Bohzok <[email protected]>

…prove tests Signed-off-by: Oleh Bohzok <[email protected]>

Signed-off-by: Oleh Bohzok <[email protected]>

rmarinn

Can we add tests that load environment variables

rmarinn · 2025-01-18T12:45:06Z

docs/cedarling/cedarling-properties.md

+// OR from environment variables
+let config =
+    BootstrapConfig::from_raw_config_and_env(None).unwrap();
+


So if None is passed, it will just used defaults... should we also provide a function where the behavior is like passing in None?

Suggested change

// OR from environment variables

let config =

BootstrapConfig::from_raw_config_and_env(None).unwrap();

// Load the bootstrap config from the environment variables. Properties that are not defined will be assigned a default value.

let config = BootstrapConfig::from_env().unwrap();

// Load the bootstrap config from the environment variables and a given config.

let config = BootstrapConfig::from_raw_config_and_env(None).unwrap();

If passed None, will be used values from env. And if some values are not present in env it does not get values from config structure.
Ok I will add additional method from_env

Resolved in 870d869

rmarinn · 2025-01-18T12:56:42Z

jans-cedarling/cedarling/src/bootstrap_config/decode.rs

+    /// Construct `BootstrapConfig` from environment variables and `BootstrapConfigRaw` config
+    //
+    // Simple implementation that map input structure to JSON map
+    // and map environment variables with prefix `CEDARLING_` to JSON map. And merge it.


Also document which config source has a higher priority here.

I don't really get. But environment variables have bigger priority. The comment was updated.

jans-cedarling/cedarling/src/bootstrap_config/decode.rs

jans-cedarling/cedarling/src/bootstrap_config/json_util.rs

rmarinn · 2025-01-18T14:00:47Z

jans-cedarling/cedarling/src/bootstrap_config/json_util.rs

+/// Helper function to convert Python-style list strings to JSON format
+fn to_json(s: &str) -> Option<Value> {


a better name might be parse_python_list_to_json

Suggested change

/// Helper function to convert Python-style list strings to JSON format

fn to_json(s: &str) -> Option<Value> {

/// Helper function to convert Python-style list strings to JSON format

fn parse_python_list_to_json(s: &str) -> Option<Value> {

This function parse string to json. And fix some kind of string to parse correctly.
I will think about better comment.
parse_python_list_to_json doesn't look appropriate..

Improved comments in the 99859c7

…aling-issue-10648 Signed-off-by: Oleh Bohzok <[email protected]>

Signed-off-by: Oleh Bohzok <[email protected]>

…or_parse_string_as_json` Signed-off-by: Oleh Bohzok <[email protected]>

…env_vars` Signed-off-by: Oleh Bohzok <[email protected]>

Signed-off-by: Oleh Bohzok <[email protected]>

SafinWasi

The python changes look good to me, though I am concerned about the use of environment variables for a docker image. For docker, environment variables are used like so:

docker run -e <variable1> -e <variable2> ...

This is not feasible for a massive number of environment variables that would be needed for cedarling.
In addition docker-compose.yml needs to be updated.

SafinWasi · 2025-01-22T16:53:06Z

jans-cedarling/flask-sidecar/main/config.py

-        exit()
-    with open(CEDARLING_BOOTSTRAP_CONFIG_FILE, "r") as f:
-        CEDARLING_BOOTSTRAP_CONFIG = f.read()
+        logger.warning(


CEDARLING_BOOTSTRAP_CONFIG may be an unbound variable here. The correct way is to initialize it to a null value before the conditional.

CEDARLING_BOOTSTRAP_CONFIG = None #... if CEDARLING_BOOTSTRAP_CONFIG_FILE is None:

…ass property with None Signed-off-by: Oleh Bohzok <[email protected]>

Signed-off-by: Oleh Bohzok <[email protected]>

SafinWasi

Looks good to me

olehbozhok added 9 commits January 17, 2025 18:27

chore(jans-cedarling): implement custom deserializer `fallback_deseri…

bf2344c

…alize` that allows to deserialize struct from string Signed-off-by: Oleh Bohzok <[email protected]>

feat(jans-cedarling): add to BootstrapConfigRaw way to load from env

cbfe9f1

Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): add loading Cedarling from env

625afff

Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): improve fallback_deserialize function and im…

4540f8c

…prove tests Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): add to python BootstrapConfig method from_env

4e65439

Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): update python docs and examples

17934e4

Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): update sidecar to load Cedaring from env vars

2419b7c

Signed-off-by: Oleh Bohzok <[email protected]>

docs(jans-cedarling): update docs

fb1eb47

Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): fix clippy issues

90a3a81

Signed-off-by: Oleh Bohzok <[email protected]>

olehbozhok requested review from SafinWasi, duttarnab and rmarinn January 18, 2025 00:13

olehbozhok self-assigned this Jan 18, 2025

mo-auto added area-documentation Documentation needs to change as part of issue or PR comp-docs Touching folder /docs comp-jans-cedarling Touching folder /jans-cedarling kind-feature Issue or PR is a new feature request labels Jan 18, 2025

olehbozhok removed area-documentation Documentation needs to change as part of issue or PR comp-docs Touching folder /docs labels Jan 18, 2025

olehbozhok requested review from djellemah, nynymike and abaghinyan January 18, 2025 00:15

olehbozhok changed the title ~~feat(jans-cedarling): Load bootsrap properties from enviroment variables~~ feat(jans-cedarling): Load bootstrap properties from environment variables Jan 18, 2025

mo-auto added area-documentation Documentation needs to change as part of issue or PR comp-docs Touching folder /docs labels Jan 18, 2025

olehbozhok added 2 commits January 18, 2025 02:17

chore(jans-cedarling): fix rust tests

58fd69f

Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): fix wasm

b2df993

Signed-off-by: Oleh Bohzok <[email protected]>

olehbozhok force-pushed the jans-cedaling-issue-10648 branch from c046d85 to b2df993 Compare January 18, 2025 00:33

Merge branch 'main' into jans-cedaling-issue-10648

d390d9e

rmarinn reviewed Jan 18, 2025

View reviewed changes

olehbozhok added 11 commits January 20, 2025 16:28

Merge commit '040ff17942019bc10433ce17d819b8d8474f13c8' into jans-ced…

a53c527

…aling-issue-10648 Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): improve comments to to_json function

54895de

Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): rename fallback_deserialize to `deserialize_…

cffa5ac

…or_parse_string_as_json` Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): rename cedarling_env_vars to `get_cedarling_…

03841ad

…env_vars` Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): improve comments for from_raw_config_and_env

99859c7

Signed-off-by: Oleh Bohzok <[email protected]>

feat(jans-cedarling): add to BootstrapConfig::from_env method

95e52b1

Signed-off-by: Oleh Bohzok <[email protected]>

docs(jans-cedarling): improve doc with loading bootstrap config from env

870d869

Signed-off-by: Oleh Bohzok <[email protected]>

Merge branch 'main' into jans-cedaling-issue-10648

78cf11e

chore(jans-cedarling): add test cases for load raw config environment

581745a

Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): refactor raw config

eccab0b

Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): fix test cases

452b8c3

Signed-off-by: Oleh Bohzok <[email protected]>

olehbozhok requested a review from rmarinn January 21, 2025 22:19

rmarinn previously approved these changes Jan 22, 2025

View reviewed changes

Merge branch 'main' into jans-cedaling-issue-10648

af19b04

olehbozhok enabled auto-merge (squash) January 22, 2025 15:41

SafinWasi requested changes Jan 22, 2025

View reviewed changes

olehbozhok added 2 commits January 22, 2025 22:02

chore(jans-cedarling): add initialize CEDARLING_BOOTSTRAP_CONFIG cl…

5b9a4be

…ass property with None Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): update docker composer file for sidecar

3ffafc7

Signed-off-by: Oleh Bohzok <[email protected]>

olehbozhok dismissed rmarinn’s stale review via 3ffafc7 January 22, 2025 20:21

chore(jans-cedarling): revert all changes related to the sidecar

920a7ec

Signed-off-by: Oleh Bohzok <[email protected]>

SafinWasi approved these changes Jan 22, 2025

View reviewed changes

olehbozhok requested a review from rmarinn January 22, 2025 20:41

rmarinn approved these changes Jan 22, 2025

View reviewed changes

Merge branch 'main' into jans-cedaling-issue-10648

a350979

olehbozhok merged commit d7200cb into main Jan 22, 2025
1 check passed

olehbozhok deleted the jans-cedaling-issue-10648 branch January 22, 2025 20:46

SafinWasi mentioned this pull request Jan 24, 2025

feat(jans-cedarling): Update sidecar to allow loading environment variables #10732

Closed

6 tasks

mo-auto mentioned this pull request Feb 10, 2025

chore(main): release 1.4.0 #10818

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(jans-cedarling): Load bootstrap properties from environment variables #10692

feat(jans-cedarling): Load bootstrap properties from environment variables #10692

olehbozhok commented Jan 18, 2025

rmarinn left a comment

rmarinn Jan 18, 2025

olehbozhok Jan 20, 2025

olehbozhok Jan 20, 2025

rmarinn Jan 18, 2025

olehbozhok Jan 20, 2025 •

edited

Loading

rmarinn Jan 18, 2025

olehbozhok Jan 20, 2025

olehbozhok Jan 20, 2025

SafinWasi left a comment

SafinWasi Jan 22, 2025

SafinWasi left a comment

-// OR from environment variables
-let config =
-    BootstrapConfig::from_raw_config_and_env(None).unwrap();
+// Load the bootstrap config from the environment variables. Properties that are not defined will be assigned a default value.
+let config = BootstrapConfig::from_env().unwrap();
+// Load the bootstrap config from the environment variables and a given config.
+let config = BootstrapConfig::from_raw_config_and_env(None).unwrap();

		/// Helper function to convert Python-style list strings to JSON format
		fn to_json(s: &str) -> Option<Value> {

feat(jans-cedarling): Load bootstrap properties from environment variables #10692

feat(jans-cedarling): Load bootstrap properties from environment variables #10692

Conversation

olehbozhok commented Jan 18, 2025

Prepare

Description

Target issue

Implementation Details

Test and Document the changes

rmarinn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olehbozhok Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SafinWasi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SafinWasi left a comment

Choose a reason for hiding this comment

olehbozhok Jan 20, 2025 •

edited

Loading