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

added eval of ${var} variables in config templates #43

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

skyway777
Copy link

@skyway777 skyway777 commented Jan 31, 2018

Now you can add ENV variables or another variables to config.
For example, you want to add build number to version.
You set BUILD_NUMBER to ENV variable, and then write into your config ${process.env.BUILD_NUMBER}

Now, version of config

var config = {
    "groupId"      : "com.example",    // required - the Maven group id.
    "artifactId"   : "{name}",         // the Maven artifact id.
    "buildDir"     : "dist",           // project build directory.
    "finalName"    : "{name}",         // the final name of the file created when the built project is packaged.
    "type"         : "war",            // type of package. "war" or "jar" supported.
    "fileEncoding" : "utf-8",          // file encoding when traversing the file system, default is UTF-8
    "generatePom"  : true,             // generate a POM based on the configuration
    "pomFile"      : "pom.xml",        // use this existing pom.xml instead of generating one (generatePom must be false)
    "version": "{version}.${process.env.BUILD_NUMBER}-${process.env.NODE_ENV}",
    "repositories" : [                 // array of repositories, each with id and url to a Maven repository.
      {
        "id": "example-internal-snapshot",
        "url": "http://mavenproxy.example.com/example-internal-snapshot/"
      },
      {
        "id": "example-internal-release",
        "url": "http://mavenproxy.example.com/example-internal-release/"
      }
    ]
};

will produce version: 1.0.1.234-production

@coveralls
Copy link

coveralls commented Jan 31, 2018

Coverage Status

Coverage increased (+0.06%) to 98.077% when pulling ab5b1e9 on skyway777:master into 28c6979 on finn-no:master.

@micha149
Copy link
Contributor

micha149 commented Feb 1, 2018

Why not simply using native template-strings?

For example:

const config = {
    // ...
    version: `{version}.${process.env.BUILD_NUMBER}-${process.env.NODE_ENV}`,
    // ...
};

@skyway777
Copy link
Author

Because you cannot use this syntax in .json configs and cli-tools like https://github.com/wallaroo/maven-deploy-cli/blob/master/cli.js

@micha149
Copy link
Contributor

I understand. But I have a bad feeling about using eval. On this way you could inject any code which is executed when someone wants to "deploy" a package.

What about something like:

const config = {
    // ...
    version: `{version}.{env(BUILD_NUMBER)}-{env(NODE_ENV)}`,
    // ...
};

So, we can parse the content in curly braces. if its something like env\((.+)\) we pick it from process.env. For backwards compatibility other strings are resolved via package.json.

@skyway777
Copy link
Author

Looks good. I will change my PR that way.

@micha149
Copy link
Contributor

Alternatively we may use something like mustache or lodash.template for supporting variables in config strings. So, we could have full control over the context and adding more data would be easy.

A proper context could look like:

const context = {
    ... pkg, // merge values from package.json for backwards compatibility
    package,
    env: process.env
};

// ...

const config = {
    // ...
    version: '{{package.version}}.{{env.BUILD_NUMBER}}-{{env.NODE_ENV}}'
    // ...
}

With this solution we dont't need to implement an own templating system…

@skyway777
Copy link
Author

Added mustache in my last commit.

test.js Outdated
const EXPECTED_ARGS = [
'-Dfile=dist' + path.sep + TEST_PKG_JSON.name + '-' +
process.env.NODE_ENV +
'.'+semver.inc(TEST_PKG_JSON.version,'patch')+
Copy link
Contributor

Choose a reason for hiding this comment

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

Style: spaces around '+' and ',' are missing.

test.js Outdated
maven.config(TEST_CONFIG_WITH_ENV_VARIABLE);
maven.install();

assert.equal(process.env.NODE_ENV,'test');
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto here.

test.js Outdated
repositories: [DUMMY_REPO_SNAPSHOT, DUMMY_REPO_RELEASE],
classifier: TEST_CLASSIFIER,
generatePom: false,
'finalName': '{name}-{{env.NODE_ENV}}.{{package.version}}',
Copy link
Contributor

Choose a reason for hiding this comment

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

Style: Quoting of finalName is unneeded, and doesn't match the rest of the area.

return pkg[key];
});
obj[key] = Mustache.render(value, replacibleKeys)
.replace(/{([^}]+)}/g, function (org, key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@skyway777 have you tested if single curly braces could work with mustache? Maybe we could remove the old replacing mechanism entirely?!

Copy link
Author

@skyway777 skyway777 Feb 27, 2018

Choose a reason for hiding this comment

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

No, it doesn't support single curly braces...

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.

4 participants