Skip to content

Replace environment variable only for dab start#1641

Merged
Aniruddh25 merged 23 commits intomainfrom
fixEnvVarReplacement
Aug 24, 2023
Merged

Replace environment variable only for dab start#1641
Aniruddh25 merged 23 commits intomainfrom
fixEnvVarReplacement

Conversation

@Aniruddh25
Copy link
Copy Markdown
Collaborator

Why make this change?

  • Closes [Bug]: Enviroment Variables are expanded/inlined in the configuration file #1632
  • it's an unintended side effect of an optimisation pathway was added to the deserializer in Overhaul of config system #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 Overhaul of config system #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?

  • Integration Tests - Modified the End2EndTests, to test the commands add, update dont replace the environment variables, but start replaces it.
  • 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.

@Aniruddh25 Aniruddh25 changed the title Fix env var replacement Replace environment variable only for dab start Aug 19, 2023
@abhishekkumams
Copy link
Copy Markdown
Contributor

replaceVar should be false by default when running through CLI, but it should be true for when running directly through Engine.
I would say make it true by default, and send false from CLI in other cases. This will fix the failing tests.

Comment thread src/Config/RuntimeConfigLoader.cs Outdated
Comment thread src/Config/Converters/EntityGraphQLOptionsConverterFactory.cs
Copy link
Copy Markdown
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions

Comment thread src/Config/Converters/StringJsonConverterFactory.cs
Comment thread src/Core/Configurations/RuntimeConfigProvider.cs Outdated
Comment thread src/Config/RuntimeConfigLoader.cs
Comment thread src/Config/Converters/EnumMemberJsonEnumConverterFactory.cs
Comment thread src/Config/Converters/EntityRestOptionsConverterFactory.cs
Comment thread src/Cli/ConfigGenerator.cs Outdated
Comment thread src/Cli.Tests/EndToEndTests.cs
Comment thread src/Cli.Tests/TestHelper.cs
Copy link
Copy Markdown
Contributor

@aaronburtle aaronburtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for adding this fix!

@Aniruddh25
Copy link
Copy Markdown
Collaborator Author

Aniruddh25 commented Aug 24, 2023

replaceVar should be false by default when running through CLI, but it should be true for when running directly through Engine. I would say make it true by default, and send false from CLI in other cases. This will fix the failing tests.

Prefer to explicitly call replaceEnvVar=true since we dont want to accidentally leak any connectionstring if it happens by default.

@Aniruddh25 Aniruddh25 merged commit 854154e into main Aug 24, 2023
@Aniruddh25 Aniruddh25 deleted the fixEnvVarReplacement branch August 24, 2023 00:42
Aniruddh25 added a commit that referenced this pull request Aug 24, 2023
## 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 added a commit that referenced this pull request Aug 24, 2023
## 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]>
Aniruddh25 added a commit that referenced this pull request Aug 24, 2023
## 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 added a commit that referenced this pull request Aug 24, 2023
## Why make this change?

- Cherry-picks #1641 and #1650 to 0.8 release

---------

Co-authored-by: Sean Leonard <[email protected]>
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.

[Bug]: Enviroment Variables are expanded/inlined in the configuration file

5 participants