Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure environment variables works for child files. #2201

Merged
merged 3 commits into from
May 6, 2024

Conversation

rohkhann
Copy link
Contributor

@rohkhann rohkhann commented May 2, 2024

Why make this change?

When we are loading child config files, we were not setting replace env variable to true and hence child config environment variables were not being obeyed. This meant that when users were using @env('variable') instead of searching for the environment variable, the code took @env('vairable') as the literal string. This fix ensures that environment variables can be used for multi-db scenario.
You need to add in parent config a data-source-files referencing a child config with connection string referencing a env variable:

  1. Parent config:
    "data-source": {
    "database-type": "mssql",
    "connection-string": "@env('my-connection-string')",
    "options": {
    "set-session-context": true
    }
    },
    "data-source-files": [ "dab-config-dab.json" ]
  2. Child config named dab-config-dab.json:
    "data-source": {
    "database-type": "mssql",
    "connection-string": "@env('my-connection-string-dab')",
    "options": {
    "set-session-context": true
    }
    },

What is this change?

Adding true to runtimeconfig file.

How was this tested?

Ran an integration test with two config files and an environment variable in the child file.

@rohkhann
Copy link
Contributor Author

rohkhann commented May 2, 2024

/azp run

@seantleonard
Copy link
Contributor

Ran an integration test with two config files and an environment variable in the child file.

can this be added as integration test?

@rohkhann
Copy link
Contributor Author

rohkhann commented May 2, 2024

Ran an integration test with two config files and an environment variable in the child file.

can this be added as integration test?

Maybe bit tricky as we dont have any test structure in the pipelines right now that can take 2 config files. Given dab's ga deadline and we want this fix to go in, maybe better to invest in that testing all up for multi-db post GA.

@rohkhann
Copy link
Contributor Author

rohkhann commented May 3, 2024

/azp run

@Aniruddh25 Aniruddh25 added this to the 1.1rc milestone May 3, 2024
@seantleonard
Copy link
Contributor

PR description needs more details.

  1. Can you add a minimum reproducible example parent dab-config.json and child dab-child-config.json? (perhaps these files are already in the test project, perhaps just sample dab start command?
  2. Since there isn't an open issue tracking this, can you also note what error messages appeared when env_vars were not processed in the child config files?

Need to open issue to track adding a test: here are some notes for whoever picks it up:

  1. Test for this use case. This should be straightforward in the ConfigurationTests.cs class:
  • Set test-specific environment variable child-conf-connstring and put a value
  • create parent runtime config with data=sources property and child runtime config and write to disk (there are examples of writing custom config to disk in ConfigurationTests.cs
  • Start dab which implies that this constructor is called in RuntimeConfig.cs: public RuntimeConfig(string? Schema, DataSource DataSource, RuntimeEntities Entities, RuntimeOptions? Runtime = null, DataSourceFiles? DataSourceFiles = null)
  • Validate that whatever error message was encountered previous, is no longer experienced when not processing env vars in the child file.

@seantleonard
Copy link
Contributor

/azp run

@rohkhann rohkhann merged commit 3488493 into main May 6, 2024
7 checks passed
@rohkhann rohkhann deleted the rohkhann/EnvironmentVariableFix branch May 6, 2024 16:53
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.

None yet

4 participants