Skip to content

add support for reading credentials from environment variables in @MavenRepository #240

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

Closed
wants to merge 2 commits into from

Conversation

leonschreuder
Copy link
Contributor

In #214 support for credentials in @mavenrepository was added. This PR extends that functionality to optionally provide an environment variable in stead of the value directly.

The implementation uses the user="{{VAR}}" syntax. It would have been nicer to support literal script calls, like user="${System.getEnv("VAR")}", but this would require writing a custom templating engine (see https://stackoverflow.com/a/52183931/3968618) and compiling the code, but that is a lot of work for variable replacement and I also felt it would be risky to allow script injection here. On the other hand, using the basic form of kotlin variable syntax (e.g. "$ENV_VAR") but not any of the others (e.g. with logic in "${}") would be confusing for users.
Therefore I decided that a custom syntax would be the best option here, but I am open to feedback.

forrestbice added a commit to forrestbice/kscript that referenced this pull request Oct 12, 2019
Following from: kscripting#240
commits/d593d97c0db19b0f18aef6dc7fb7bc030b63384f.
holgerbrandl pushed a commit that referenced this pull request Oct 20, 2019
* Create a default Run Configuration for Idea

Create a default Run Configuration for Intellij when using the `idea`
option. Achieved by creating a `Main` configuration within
`.idea/runConfigurations`. The `userArgs` are also passed along by
default.

* Fix Incorrect Path In Script Tests

Following from: #240
commits/d593d97c0db19b0f18aef6dc7fb7bc030b63384f.
@holgerbrandl
Copy link
Collaborator

Nice enhancement. Would you be willing to also add a small note to the README about this new feature? If so, I'd wait with the merge.

@holgerbrandl
Copy link
Collaborator

Duplicate of #248.

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.

2 participants