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

[FEATURE] Add support to autofill env-vars in config #764

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions lib/graph/Module.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,16 @@ function clone(obj) {
* @alias @ui5/project/graph/Module
*/
class Module {
/**
* Regular expression to identify environment variables in strings.
* Environment variables have to be prefixed with "env:", e.g. "env:Path".
*
* @private
* @static
* @readonly
*/
static _ENVIRONMENT_VARIABLE_REGEX = /env:\S+/g;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static _ENVIRONMENT_VARIABLE_REGEX = /env:\S+/g;
static _ENVIRONMENT_VARIABLE_REGEX = /^env:\S+$/g;

Match the start and end of the string to prevent partial substitution like "env:should" in this env:should not be replaced.

At least I think that would be unwanted/unexpected by most users. Or did you expect this to work as well?

Copy link
Author

Choose a reason for hiding this comment

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

I considered this aswell, but figured this would actually be expected behavior since environment variables behave like this in batch and powershell script string interpolation.

Consider a custom task that takes a configuration value in a specific format, e.g. something like

configuration:
      example1: "key1=value1;key2=value2"
      example2: "value2"

With the current implementation, the user could inject their environment variables however they might like, e.g. something like

configuration:
      example1: "key1=env:myvalue;key2=value2"
      example2: "env:myvalue"

This approach allows a more fine-grained configuration IMO. Now one might argue that the dev should not have implemented their custom configuration like this in the first place (and I agree, at least for this example a yaml array would have sufficed), but then again the goal is to move the responsibility away from the dev.

I think it's very unlikely that a user might have a substring in a config value that coincidentally matches the environment variable regex given a sufficiently unique env var format, so the additional control and flexibility the user gets in return seems worth it.

I'll gladly change it if you want me to, though


/**
* @param {object} parameters Module parameters
* @param {string} parameters.id Unique ID for the module
Expand Down Expand Up @@ -339,6 +349,25 @@ class Module {
return configs;
}

try {
for (const config of configs) {
if (config.customConfiguration !== undefined) {
this._normalizeConfigValue(config, "customConfiguration");
}
if (config.builder?.customTasks !== undefined) {
this._normalizeConfigValue(config.builder, "customTasks");
}
if (config.server?.customMiddleware !== undefined) {
this._normalizeConfigValue(config.server, "customMiddleware");
}
}
} catch (err) {
throw new Error(
"Failed to parse configuration for project " +
`${this.getId()} at '${configPath}'\nError: ${err.message}`
);
}

// Validate found configurations with schema
// Validation is done again in the Specification class. But here we can reference the YAML file
// which adds helpful information like the line number
Expand Down Expand Up @@ -406,6 +435,45 @@ class Module {
return config;
}

/**
* Normalizes the config value at object[key]. If the config value is an object / array,
* this method will descend depth-first on its properties / elements. If the config value is a string
* and contains any environment variables, they will be filled in at this point.
*
* @private
* @param {(object|any[])} object An object or array
* @param {(string|number)} key A key of the object or an index of the array
*/
_normalizeConfigValue(object, key) {
let value = object[key];
switch (typeof value) {
case "string": {
value = value.replace(Module._ENVIRONMENT_VARIABLE_REGEX, (substring) => {
const envVarName = substring.slice(4);
if (process.env[envVarName] === undefined) {
throw new Error(`The environment variable '${envVarName}' is not defined`);
}
return process.env[envVarName];
});
object[key] = value;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether this requires additional checks to prevent prototype pollution, since the source of the configuration might not be fully trusted.

In case the configuration object has been loaded from a YAML, the yaml-js module already used defineProperty for any __proto__ keys when creating the object. I'm not sure whether we still need to do the same when updating the property's value.

break;
}
case "object": {
if (value === null) break;
if (Array.isArray(value)) {
for (let i = 0; i < value.length; i++) {
this._normalizeConfigValue(value, i);
}
} else {
for (const key of Object.keys(value)) {
this._normalizeConfigValue(value, key);
}
}
break;
}
}
}

/**
* Resource Access
*/
Expand Down
25 changes: 25 additions & 0 deletions test/fixtures/application.a/ui5-test-env.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
---
specVersion: "3.0"
type: application
metadata:
name: application.a
customConfiguration:
stringWithEnvVar: env:testEnvVarForString
objectWithEnvVar:
someKey: env:testEnvVarForObject
arrayWithEnvVar:
- a
- env:testEnvVarForArray
- c
builder:
customTasks:
- name: foo
afterTask: replaceVersion
configuration:
builderWithEnvVar: env:testEnvVarForBuilder
server:
customMiddleware:
- name: bar
afterMiddleware: compression
configuration:
serverWithEnvVar: env:testEnvVarForServer
71 changes: 71 additions & 0 deletions test/lib/graph/Module.js
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,77 @@ test("Legacy patches are applied", async (t) => {
.map(testLegacyLibrary));
});

test.serial("Environment variables in configuration", async (t) => {
const testEnvVars = [
"testEnvVarForString",
"testEnvVarForObject",
"testEnvVarForArray",
"testEnvVarForBuilder",
"testEnvVarForServer",
].map((testEnvVar, index) => {
const wrapper = {
name: testEnvVar,
oldValue: process.env[testEnvVar],
newValue: `testValue${index}`,
};
process.env[testEnvVar] = wrapper.newValue;
return wrapper;
});
try {
const ui5Module = new Module({
id: "application.a.id",
version: "1.0.0",
modulePath: applicationAPath,
configPath: "ui5-test-env.yaml",
});
const {project} = await ui5Module.getSpecifications();
t.deepEqual(
project.getCustomConfiguration(),
{
stringWithEnvVar: testEnvVars[0].newValue,
objectWithEnvVar: {
someKey: testEnvVars[1].newValue,
},
arrayWithEnvVar: ["a", testEnvVars[2].newValue, "c"],
},
"Environment variables in custom configuration are filled in"
);
t.deepEqual(
project.getCustomTasks()?.[0]?.configuration,
{
builderWithEnvVar: testEnvVars[3].newValue,
},
"Environment variable in builder custom tasks is filled in"
);
t.deepEqual(
project.getCustomMiddleware()?.[0]?.configuration,
{
serverWithEnvVar: testEnvVars[4].newValue,
},
"Environment variable in server custom middleware is filled in"
);
} finally {
// Reset all env vars back to their value previous to testing
testEnvVars.forEach((wrapper) => {
if (wrapper.oldValue === undefined) {
delete process.env[wrapper.name];
} else {
process.env[wrapper.name] = wrapper.oldValue;
}
});
}
});

test.serial("Undefined environment variables in configuration", async (t) => {
const ui5Module = new Module({
id: "application.a.id",
version: "1.0.0",
modulePath: applicationAPath,
configPath: "ui5-test-env.yaml",
});
await t.throwsAsync(() => ui5Module.getSpecifications());
});

test("Invalid configuration in file", async (t) => {
const ui5Module = new Module({
id: "application.a.id",
Expand Down