Skip to content

Add "Authentication from environment" #248

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

Merged
merged 8 commits into from
Nov 22, 2019
Merged

Add "Authentication from environment" #248

merged 8 commits into from
Nov 22, 2019

Conversation

alekseytomin
Copy link

I have to use the private nexus repository in gitlab-ci.
We use only authenticated access to it. But I mustn't write a password in scripts.

PS: I've chosen '@' as a symbol ('$' isn't possible in Kotlin). Maybe You will choose another one.

PPS: I have not found any other point to use environment variables.

@holgerbrandl
Copy link
Collaborator

Great suggestion. I would also never include credentials in scripts.

Indeed @ feels a bit odd. Maybe {{MY_PW}} could feel a bit more templaty?

I think before we should merge it, the fallback behaviour when a variable is not defined should be changed to make kscript stop script execution. Tis may clash with the rare cases that a pw is actually starting with {{ and ends with }}, but it seems preferable to be than using wrong credentials, just because the user forgot to export the variables in bash before invoking kscript.. What do you think?

@alekseytomin
Copy link
Author

alekseytomin commented Nov 15, 2019

Great suggestion. I would also never include credentials in scripts.

Indeed @ feels a bit odd. Maybe {{MY_PW}} could feel a bit more templaty?

Ok. I've forgotten this.

I think before we should merge it, the fallback behaviour when a variable is not defined should be changed to make kscript stop script execution.

System.getenv() in Kotlin is a nonnull function.
Without any changes we have an exception:

Exception in thread "main" java.lang.IllegalStateException: System.getenv(value.subs…ing(2, value.length - 2)) must not be null
	at kscript.app.DependencyUtilKt.decodeEnv(DependencyUtil.kt:72)
...

I think better to write something like

Exception in thread "main" java.lang.RuntimeException: environment variable 'HEXUS_USER' doesn't found
	at kscript.app.DependencyUtilKt.decodeEnv(DependencyUtil.kt:76)
...

Or maybe hide the stacktrace?

@holgerbrandl
Copy link
Collaborator

holgerbrandl commented Nov 15, 2019

3 minor aspects (which I typically would just do myself, but I'm a bit in a hurry this week)

  1. In the docs the example pw artifactory_password should be uppercase ARTIFACTORY_PASSWORD since bash variables are typically uppercase
  2. I like the more custom error. But the wording seems slightly wrong. Couldn't we just say

"Could not resolve environment variable {{bla}} in maven repository credentials"

  1. Instead of rethrowing the exception, we should better exit with
errorMsg(lazyMessage().toString())
quit(1)

This will prefix the message with [kscript] [ERROR] which will help the user to distinguish script logic errors from kscript runtime errors.

@alekseytomin
Copy link
Author

Done

@alekseytomin
Copy link
Author

c8fc911
I've found than System.getenv(key) has a wrong type (String! instead of String?) and remove from code. System.getenv()[key] returns String?

@alekseytomin
Copy link
Author

Is it okay?

@holgerbrandl holgerbrandl merged commit aac75af into kscripting:master Nov 22, 2019
@holgerbrandl
Copy link
Collaborator

Great PR, thanks for your support and patience. A release including your feature is planned for later today.

@alekseytomin
Copy link
Author

Thanks!

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