Skip to content

[CherryPick]: Replace environment variable only for dab start#1652

Merged
Aniruddh25 merged 2 commits intorelease/0.8from
cherryPickreplaceEnvVarFix
Aug 24, 2023
Merged

[CherryPick]: Replace environment variable only for dab start#1652
Aniruddh25 merged 2 commits intorelease/0.8from
cherryPickreplaceEnvVarFix

Conversation

@Aniruddh25
Copy link
Copy Markdown
Collaborator

Aniruddh25 and others added 2 commits August 24, 2023 15:40
## Why make this change?

- Closes #1632
- it's an unintended side effect of an optimisation pathway was added to
the deserializer in #1402.
Previously, deserializing the JSON config and replacing environment
variables were done as two separate parses of the config JSON blob.
First the JSON would be read from disk and then we'd create a JSON
reader that would walk all the tokens in the file, we'd check the token
to see if it matched the regex for an environment variable, we'd replace
it and then return the updated JSON string. Next this would be given to
the deserializer and it would be turned into the .NET object that we
would use throughout the engine. This meant that we'd end up walking the
AST for the JSON twice.
- With #1402, when we are doing deserialization we would also do the
environment variable replacement, meaning that we'd only walk the JSON
AST once, thus improving performance.
- There is a downside to this - the CLI doesn't need to do environment
variable replacements (since we don't need to actually connect to the DB
or anything, except for start, but that doesn't write out an updated
config).

## What is this change?

- Introduces a flag replaceEnvVar to determine whether to do the
environment variable replacement in the JsonConverters when being used
from the CLI commands. For `start` command this will be `true`, for all
other commands this is `false`.
- To minimize changes, default value of this argument was kept false.
All occurrences of callers requiring this flag were modified and set to
`true` in case of `start` or running the engine.
- For Enums, always replace the environment variable with its value for
appropriate deserialization into an Enum value.

## How was this tested?

- [X] Integration Tests - Modified the End2EndTests, to test the
commands `add`, `update` dont replace the environment variables, but
`start` replaces it.
- [X] Unit Tests - Modified the `CheckConfigEnvParsingTest` to test when
replaceEnvVar is `false` we dont replace them. The `true` scenario was
already being tested. Also modified the `database-type` property to be
an env variable for testing enums are always replaced with values.

---------

Co-authored-by: Sean Leonard <[email protected]>
## Why make this change?

- Accidentally completed the PR #1641 before pushing the last commit
that addressed review comments. I had it on my local branch, but forgot
to push before merging in. This PR is to push those left over review
comments into main.
@Aniruddh25 Aniruddh25 merged commit e201e6d into release/0.8 Aug 24, 2023
@Aniruddh25 Aniruddh25 deleted the cherryPickreplaceEnvVarFix branch August 24, 2023 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants