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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
174b695
[FEATURE] Add support to autofill env-vars in config
menof36go Aug 1, 2024
ea07ad7
Update test/lib/graph/Module.js
menof36go Sep 1, 2024
1f8f496
[FEATURE] Only transform env vars in customConfiguration, customMW, c…
menof36go Sep 30, 2024
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
49 changes: 49 additions & 0 deletions lib/graph/Module.js
View file
Open in desktop
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 @@ -403,9 +413,48 @@ class Module {
if (!config.kind) {
config.kind = "project"; // default
}
for (const key of Object.keys(config)) {
this._normalizeConfigValue(config, key);
}
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);
return process.env[envVarName] || substring;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure whether we should do this fallback in case the environment variable is empty or not set.

I would expect that we always replace the env-pattern, since the pattern itself is probably never the correct value for whatever you are passing it to. An empty value seems more likely to trigger checks and result in a reasonable error for the user (i.e. "parameter xyz is empty" from a custom task).

I'll have to check again, but I think that would be in line with other tools like Docker as well.
From dotenv:

empty values become empty strings (EMPTY= becomes {EMPTY: ''})

I'm also wondering whether we should differentiate between empty environment variables and variables that are not set at all. I'm leaning towards treating them the same, but we should check how other tools are handling this.

Copy link
Author

@menof36go menof36go Sep 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, I agree that the current fallback is bad.
We have to consider that the task has no way of knowing that the user might have forgotten something since it doesn't and shouldn't know about the auto-fill. So there are 2 possible outcomes:

  1. empty string is a legal value for the configuration key's value
  2. empty string is an illegal value for the configuration key's value

In case 2, we are fine, the task will (hopefully) throw an error.

But what about case 1?
In case 1 the task may not fail at all and the results may be very different from what the user expects.
Imagine how hard it might be for a user to figure out why their results are all messed up in this case, when seemingly no error occurred.

For this reason, I think it would be best if we take responsibility and throw a reasonable error ourselves instead of delegating it to the task. At the very least we should warn the user imo.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, we can't tell whether an empty string is a legal value for a given configuration option.

Are you proposing to raise an error in case an env-var is not set at all, or also in case it is set to an empty string? By now I think I'm more in favor of the first one, since empty strings may be valid configuration options.

I understand that we both agree the name of the environment variable should never be used as a value.

Copy link
Author

Choose a reason for hiding this comment

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

Are you proposing to raise an error in case an env-var is not set at all, ... By now I think I'm more in favor of the first one, since empty strings may be valid configuration options.

Yes, exactly, I think it is the best way to avoid unexpected behavior

I understand that we both agree the name of the environment variable should never be used as a value.

Yes, I agree

Copy link
Member

Choose a reason for hiding this comment

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

After discussing this with some colleagues, I'm not sure whether raising an expection will work well with some of the expectations from SAP/ui5-tooling#935.

Especially when environment variables are used in (transitive-)dependencies (e.g. a reuse-library), you would have to make sure to set all those variables when building the root project, as otherwise you will face an error.

  1. For things like enabling the debug mode of a custom task, this doesn't seem ideal. Surely you don't want to always have to set DEBUG= to explicitly disable the debug mode. I'd rather expect the debug mode to be disabled by default, unless DEBUG=true is set.
  2. If a dependency adds an environment variable in a new release, your project's build would break after updating the dependency, until you also provide the new environment variable (which might be something irrelevant for your project)

Copy link
Author

Choose a reason for hiding this comment

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

After discussing this with some colleagues, I'm not sure whether raising an expection will work well with some of the expectations from SAP/ui5-tooling#935.

Especially when environment variables are used in (transitive-)dependencies (e.g. a reuse-library), you would have to make sure to set all those variables when building the root project, as otherwise you will face an error.

1. For things like enabling the debug mode of a custom task, this doesn't seem ideal. Surely you don't want to always have to set `DEBUG=` to _explicitly disable the debug mode_.  I'd rather expect the debug mode to be disabled by default, _unless_  `DEBUG=true` is set.

2. If a dependency adds an environment variable in a new release, your project's build would break after updating the dependency, until you also provide the new environment variable (which might be something irrelevant for your project)

I agree, environment variables inside your dependencies yamls sound like a hot mess, I will gladly give that up (see comment below). If we do however restrict the feature to the root project, would you then agree that an exception is the way to go? The only way for an exception to pop up in that case is if the user messed up their own config and updates to dependencies should no longer be a concern

});
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
56 changes: 56 additions & 0 deletions test/lib/graph/Module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,62 @@ test("Legacy patches are applied", async (t) => {
.map(testLegacyLibrary));
});

test("Environment variables in configuration", async (t) => {
menof36go marked this conversation as resolved.
Show resolved Hide resolved
const testEnvVars = ["testEnvVarForString", "testEnvVarForObject", "testEnvVarForArray"].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,
configuration: {
specVersion: "2.6",
type: "application",
metadata: {
name: "application.a-object",
},
customConfiguration: {
stringWithEnvVar: `env:${testEnvVars[0].name}`,
Copy link
Member

Choose a reason for hiding this comment

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

Please also add a case where the pattern is inside a string (see my comment on the regex)

objectWithEnvVar: {
someKey: `env:${testEnvVars[1].name}`,
},
arrayWithEnvVar: ["a", `env:${testEnvVars[2].name}`, "c"]
},
},
});
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 variable 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("Invalid configuration in file", async (t) => {
const ui5Module = new Module({
id: "application.a.id",
Expand Down