-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Variable substitution in DataSource JSON files #2556
Comments
@bajtos , I was analyzing the datasource booter and found that this can be done before it calls the repositoryMixin.dataSource easily. I am suggesting that we use a convention based on the datasource name, without specifying any variable in the .json file. // src/datasources/db.json
{
"name": "myDBName",
"connector": "mysql",
"host": "xxxx",
"port": "yyyy",
"user": "xxxxxkkd",
...
} Then in the thoughts? |
we need a JSON utility that can perform dependency injection |
The team brought up a few questions:
We figure we may need a file format different from .json. Perhaps .config. Please clarify the Acceptance Criteria in light of our comments. Thank you. |
@bajtos ^ |
@marioestradarosa Thank you for chiming in!
Yes, that was my roughly my idea too.
I see this as an alternative/complementary solution to what I have originally proposed. In many environments, the application developer does not control the environment variables. For example, Heroku uses
Good question! I am proposing to use the following syntax for numeric values (notice the // a number
"port": "${+ENV.PORT}",
// a string
"host": "${ENV.HOST}", Compare this to regular javascript: const port = +'0';
const host = 'localhost';
That's out of scope of this story. When the environment value is not set, then the config property is set to
That's out of scope of this story. Please open a new issue to discuss this idea. |
Updated the acceptance criteria:
|
Hmm, I was looking at the current implementation of datasource booter and AFAICT, we are not processing datasource JSON files at boot time right now. We rely on the TypeScript source to load the configuration from the As I was envisioning this story, I wanted only a small tweak in the datasource booter to perform variable substitution on the configuration data loaded from JSON before the config is bound to context. That's obviously not possible in the current architecture. In that light, I think this story is not ready to be worked on and I'll need to think a bit more about how to approach the problem of modifying datasource config in production. |
Getting into the LB4 ecosystem Wanted to point out that loading everything from the environment is also not ideal -- lot's of ways that environment values can be leaked -- ideal would also support loading from some kind of happy to share some snippets once I get a working solution |
Good point. I am not very familiar with Kubernetes, Istio and similar tools, so pardon me if my question is stupid. But isn't it the responsibility of the container orchestration tool to fetch the configuration from a valt into environment variables before starting the process? Having said that, if it makes sense for the app to load the config from the vault directly, and we can find an elegant solution, then I am fine with implementing such feature ✌️ |
Pulling directly from a vault is also likely not ideal since there are so many to choose from, this would be more appropriate to hand off to a plugin or easy hook method. My recommendation would be to using a cascade method with an easy method of providing an callback ie:
make sure the config structure is well documented for the different locations it will look for make sense? |
Recently, I am leaning towards a different approach. I think we should remove The idea is to embed the default datasource configuration directly inside import {inject} from '@loopback/core';
import {juggler} from '@loopback/repository';
export const CONFIG = {
name: 'db',
connector: 'memory',
localStorage: '',
file: './data/db.json',
};
export class DbDataSource extends juggler.DataSource {
static dataSourceName = 'db';
constructor(
@inject('datasources.config.db', {optional: true})
dsConfig: object = CONFIG,
) {
super(dsConfig);
}
} With this new design in place, variable substitution does not require any special support from LoopBack. export const CONFIG = {
name: 'db',
connector: 'mysql',
// in production
url: process.env.MYSQL_CONNECTION_STRING,
// local dev & test
database: 'myapp',
}; Obviously, this does not solve the requirement that |
See #5000. I am closing this issue in favor of #5000 and #1464. The current solution for 12factor env-based configuration is described in #1464 (comment):
|
To support https://12factor.net/ application loading their database connection strings from ENV variables, we should enhance our DataSource booter to provide variable substitution similar to what https://github.com/strongloop/loopback-boot already provides for LB3 applications.
Example LB3 config file:
Proposed format for LB4:
By using
ENV.
prefix, we are keeping doors open to add other variable sources in the future, e.g.APP.
prefix to access application-level bindings.To support numeric values, allow the following format (notice the
+
sign after the opening bracket):Acceptance criteria
{ENV: process.env}
), and returning modified configuration object with substitutions resolved according to rules described above. Include comprehensive test coverage.@loopback/boot
recognizes and replaces variables in datasource configuration. Leverage the already implemented helper function.process.env
must not be accessed directly from individual booters. Instead, the environment object should be provided viaapp.boot()
arguments. For example, we can add a new property toBootExecutionOptions
.process.env
value toapp.boot()
, so that variable substitution can be performed.Out of scope
The text was updated successfully, but these errors were encountered: