Fix and improve environment variable resolution in repositories

[rocketraman]

Resolves #402.

The manual tests for an authenticated repo were not complete.

Fixed and improved the test artifactory server startup.

Added a manual test for environment variable substitution (test 4).

Added a manual test for environment variable substitution with packaging (test 5).

Fix docs for env var substitution to use $FOO rather than {{FOO}} to be compatible with the Kotlin Script backend.

[aartiPl]

The PR looks really nice now. But, I have some additional improvement ideas that will make it even better. Please have a look at my comment inline.
Additionally, there is a failing test for IntelliJ in the test suite. Please have a look at the "Checks" tab.

[aartiPl]

• Please merge the two functions (resolveRepositoryOption is enough - I understand why you have kept the initial function from Kotlin code, but it will be easy enough to handle potential future changes in the Kotlin code).

• I am trying to avoid reading env variables and config files in the app as much as possible. It makes testing classes in separation much easier. So in the parameter to resolveRepositoryOption, you can pass environment: Map<String, String?> and configMap: Map<String, String?>, which will be passed from scriptingConfig. You have a reference to scriptConfig in SectionResolver. Adding additional fields with env variables and configMap in ScriptConfig should be very easy as the env variables map is already passed to ConfigBuilder, and configProperties can be converted to configMap easily (why configMap, and not just properties? --> Properties class brings too much functionality, which should not be available after configuration phase).

• values providedRepositoryUrl, providedRepositoryUser, and providedRepositoryPassword won't be needed anymore in ScriptingConfig, because we will pass all env vars and config options. [Point for consideration: maybe we should instead pass those two maps in OsConfig, not in ScriptingConfig]

• Now, we can easily implement the following functionality:
  a) first, we will check the env variable as it is
  b) if it is not present, then we will check in the configMap entries: scripting.repository.$variable or just $variable. The first option is IMHO safer. Example: user="$artifactory.user" should be first read from env variables envKey=artifactory.user and if it is not there from configMap: scripting.repository.artifactory.user
  c) if the resolution of variables is blank, then we can throw an exception (I think it will be useful to explain why it has failed, providing in the message search keys for the env variable and for configMap)
  d) I think we can drop all the special functionality around KSCRIPT_REPOSITORY_* env variables, as the proposed solution is much more generic

• Move the merged function resolveRepositoryOption to ScriptUtils. Now it will be possible to unit test it in separation, and we will shorten the size of SectionResolver

• You can simplify tests - no additional tool is needed to test changes in env variables and config properties

• Please document the changes

[aartiPl]

It's not precisely accurate that KScript has switched to Kotlin Scripting backend. I have a branch kscript_4.4 in the repository, already converted to Kotlin Scripting. Around 50% of test cases for this branch are passing, so it is not that bad. But I do not have much time to work on it, and the Kotlin Scripting code is not straightforward and poorly documented. But maybe you can find some time to have a look at it? Work on the Kotlin Scripting integration will require close cooperation with Kotlin people.
In the long term, it is the best option for KScript and Kotlin Scripting users. KScript still provides some additional, excellent features, which will never be implemented by JetBrains but can give real benefits to the users.

[rocketraman]

Thanks for your review @aartiPl .

• Please merge the two functions (resolveRepositoryOption is enough - I understand why you have kept the initial function from Kotlin code, but it will be easy enough to handle potential future changes in the Kotlin code).

Merging the functions actually makes the code messier, because tryResolveEnvironmentVariable has multiple returns including two places where null is returned, so if the function is inlined the logic gets significantly more complex for no real gain. If this is really important for some reason, I could always nest the tryResolveEnvironmentVariable function.

I am trying to avoid reading env variables and config files in the app as much as possible. [...snip...]

For the environment part of this, I think this is a good idea. Done.

values providedRepositoryUrl, providedRepositoryUser, and providedRepositoryPassword won't be needed anymore in ScriptingConfig, because we will pass all env vars and config options. [Point for consideration: maybe we should instead pass those two maps in OsConfig, not in ScriptingConfig]

I'm not fond of this change. In general I prefer strongly typed values to loosely typed Maps. For environment, it makes sense as a Map as the environment is already external loosely typed data. But for the KSCRIPT_REPOSITORY_* vars, those are documented (at least after my PR update, lol) kscript properties with specific behaviors and types. I don't believe it makes sense to move these from being strongly typed as they are now to loosely typed maps.

This would also introduce a larger change/refactoring to ConfigBuilder which, if we decide to do it, should probably be done as a separate PR.

I'll resubmit without this, and we can continue the discussion separately.

Move the merged function resolveRepositoryOption to ScriptUtils. Now it will be possible to unit test it in separation, and we will shorten the size of SectionResolver

Ok.

Please document the changes

Ok.

PR updated. I pushed one fixup to an earlier commit for the test failure, but added new commits for all the other bigger changes.

It's not precisely accurate that KScript has switched to Kotlin Scripting backend. I have a branch kscript_4.4 in the repository, already converted to Kotlin Scripting. Around 50% of test cases for this branch are passing, so it is not that bad. But I do not have much time to work on it, and the Kotlin Scripting code is not straightforward and poorly documented. But maybe you can find some time to have a look at it? Work on the Kotlin Scripting integration will require close cooperation with Kotlin people.
In the long term, it is the best option for KScript and Kotlin Scripting users. KScript still provides some additional, excellent features, which will never be implemented by JetBrains but can give real benefits to the users.

Fair enough -- I guess I happened to run into issues with the subset that had been migrated. Agreed with the general direction of kscript + Kotlin Scripting. Unfortunately I don't think I have enough time to devote to doing this properly either.

[aartiPl]

Merging the functions actually makes the code messier, because tryResolveEnvironmentVariable has multiple returns including two places where null is returned, so if the function is inlined the logic gets significantly more complex for no real gain. If this is really important for some reason, I could always nest the tryResolveEnvironmentVariable function.

With the current approach, you have e.g. 2 places where the error is generated, which is not really good. I can merge those functions later, and refactor - no problem.

I'm not fond of this change. In general I prefer strongly typed values to loosely typed Maps. For environment, it makes sense as a Map as the environment is already external loosely typed data. But for the KSCRIPT_REPOSITORY_* vars, those are documented (at least after my PR update, lol) kscript properties with specific behaviors and types. I don't believe it makes sense to move these from being strongly typed as they are now to loosely typed maps.

That's exactly my approach - strong-typed apps are much easier to maintain. But in this case, we are not losing strong-type safety. From env variables and from config files you always get just strings. Only later they can be converted to specific types. In our case, the requirement is to resolve those values late in the process. The resolution of variables has to happen late because of the nature of the feature which we want to introduce.

KSCRIPT_REPOSITORY_* vars were introduced by me when I completely rewritten kscript, basically from scratch. It wasn't the best design decision (e.g. with such an approach you can have only one, external repository with provided user and password), but at that time it was just a minor issue, so I didn't care too much. But now it's time to redesign it and fix its problems.

This would also introduce a larger change/refactoring to ConfigBuilder which, if we decide to do it, should probably be done as a separate PR.

I'll resubmit without this, and we can continue the discussion separately.

I don't think we need another PR. Without changes, we will have a half-backed, inconsistent feature, so from that point of view it should be still the same PR:

• Inconsistent - there are two ways of accessing user variables: with {{VAR}} and with $VAR. If we want to follow Kotlin Scripting approach, there should be available only $VAR
• Half-backed - I have introduced a config file for kscript, which I think is a nice feature. It allows one to easily store some repeated properties. But with the current approach, it will be possible to set the repository URL, repository user, and repository password only from environment variables, but not from a config file.

You shouldn't be afraid of changing ConfigBuilder. It just creates Config object which consists of ScriptingConfig and OsConfig. If you pass it down to the new resolver it will be easy to follow the same logic as it is now in ConfigBuilder:

val extractedVariable = "..."
val repositoryUrl = environment.getEnvVariableOrNull(extractedVariable) ?: config.getPropertyOrNull("scripting.repository.$extractedVariable")

check(respositoryUrl.isNotBlank()) {
  "Could not resolve repository URL neither as environment variable '$extractedVariable', nor as a config property 'scripting.repository.$extractedVariable'"
}

I am not sure if I explained correctly how I would like to see the implementation. If I was not clear enough, please do not hesitate to ask.

PR updated. I pushed one fixup to an earlier commit for the test failure but added new commits for all the other bigger changes.

I meant this failure in the integration test:
https://github.com/kscripting/kscript/actions/runs/5349998123

Not sure if you can start it by yourself in Github. Anyway, integration tests can be started manually on your local Linux:

./gradlew -DosType=$OSTYPE -DincludeTags='posix | linux' integrationTest

[rocketraman]

Merging the functions actually makes the code messier, because tryResolveEnvironmentVariable has multiple returns including two places where null is returned, so if the function is inlined the logic gets significantly more complex for no real gain. If this is really important for some reason, I could always nest the tryResolveEnvironmentVariable function.

With the current approach, you have e.g. 2 places where the error is generated, which is not really good. I can merge those functions later, and refactor - no problem.

Sure you can refactor as you wish, but FYI an error is only thrown in one place. There is a second error log for the missing but referenced environment variable, which I think is a feature rather than a bug :-), as it provides additional context to the later exception.

I'm not fond of this change. In general I prefer strongly typed values to loosely typed Maps. For environment, it makes sense as a Map as the environment is already external loosely typed data. But for the KSCRIPT_REPOSITORY_* vars, those are documented (at least after my PR update, lol) kscript properties with specific behaviors and types. I don't believe it makes sense to move these from being strongly typed as they are now to loosely typed maps.

That's exactly my approach - strong-typed apps are much easier to maintain. But in this case, we are not losing strong-type safety. From env variables and from config files you always get just strings. Only later they can be converted to specific types. In our case, the requirement is to resolve those values late in the process. The resolution of variables has to happen late because of the nature of the feature which we want to introduce.

KSCRIPT_REPOSITORY_* vars were introduced by me when I completely rewritten kscript, basically from scratch. It wasn't the best design decision (e.g. with such an approach you can have only one, external repository with provided user and password), but at that time it was just a minor issue, so I didn't care too much. But now it's time to redesign it and fix its problems.

Yes, I thought that was odd.

I have no problem with this being improved, but first, we are venturing really far from the point of my PR and the issue I raised, which was to fix a bug of a documented feature of env var resolution. I didn't plan on discussing or implementing a redesign for an existing feature related to config properties.

I will note that the approach I would take here, were I to implement some feature like this, would be more "script-like" and less "application-like". What you are describing here is reminscient to me of Spring Boot's externalized configuration, with an extensive mechanism to create a single abstraction for both configuration and environment.

I'd be willing to be convinced here, but my initial thought is that this is overkill for scripts. Scripts need a way to reference the environment in order to bootstrap itself. Any other complex configuration mechanisms can always be loaded by the script itself if it needs them. Think bash, not Spring Boot. Now, obviously kscript has to provide a bit more than bash, as it needs to load dependencies and such, but IMHO the capabilities here should be minimalistic and straightforward.

What were the use cases for script config properties in the first place that env vars were unable to solve?

This would also introduce a larger change/refactoring to ConfigBuilder which, if we decide to do it, should probably be done as a separate PR.
I'll resubmit without this, and we can continue the discussion separately.

I don't think we need another PR. Without changes, we will have a half-backed, inconsistent feature, so from that point of view it should be still the same PR:

• Inconsistent - there are two ways of accessing user variables: with {{VAR}} and with $VAR. If we want to follow Kotlin Scripting approach, there should be available only $VAR

Its not inconsistent if there are two different sources: the environment and properties are not the same. Things can also be made "consistent" by getting rid of property configuration entirely :-).

• Half-backed - I have introduced a config file for kscript, which I think is a nice feature. It allows one to easily store some repeated properties. But with the current approach, it will be possible to set the repository URL, repository user, and repository password only from environment variables, but not from a config file.

Agreed, but this isn't a bug -- its the way the feature is currently designed.

You shouldn't be afraid of changing ConfigBuilder. It just creates Config object which consists of ScriptingConfig and OsConfig. If you pass it down to the new resolver it will be easy to follow the same logic as it is now in ConfigBuilder:

val extractedVariable = "..."
val repositoryUrl = environment.getEnvVariableOrNull(extractedVariable) ?: config.getPropertyOrNull("scripting.repository.$extractedVariable")

check(respositoryUrl.isNotBlank()) {
  "Could not resolve repository URL neither as environment variable '$extractedVariable', nor as a config property 'scripting.repository.$extractedVariable'"
}

I am not sure if I explained correctly how I would like to see the implementation. If I was not clear enough, please do not hesitate to ask.

Its clear enough but kind of out of scope for what I had intended for this PR, which was to fix a clear and obvious bug rather than redesign a feature. See comments above.

PR updated. I pushed one fixup to an earlier commit for the test failure but added new commits for all the other bigger changes.

I meant this failure in the integration test: https://github.com/kscripting/kscript/actions/runs/5349998123

Not sure if you can start it by yourself in Github. Anyway, integration tests can be started manually on your local Linux:

./gradlew -DosType=$OSTYPE -DincludeTags='posix | linux' integrationTest

Tried this, but the integration test is not failing for me locally on commit cab0d2f?

SUCCESS: Executed 52 tests in 2m 19s

Include tags: posix | linux
Exclude tags:

BUILD SUCCESSFUL in 2m 25s
7 actionable tasks: 7 executed

[aartiPl]

What were the use cases for script config properties in the first place that env vars were unable to solve?

It's easier to set up a config file than to set up env variables persistently. On Linux, you must modify .bashrc to have those vars always available (ironically, you must program in bash to set up Kotlin scripting). On Windows, you have to set the vars in UI manually.

The config file gives persistent setup without a hassle and allows easy configuration backups.

---

Yes, I am asking for more than the initial PR. It would be great if you could continue to work on PR, but I understand if it is too much. I may be able to work on the feature later as I am now working on another Open Source project that will be closely connected with Kotlin Scripting.

(I will be off for vacation the next few days, so I will reply after I return).

[rocketraman]

It's easier to set up a config file than to set up env variables persistently. On Linux, you must modify .bashrc to have those vars always available (ironically, you must program in bash to set up Kotlin scripting). On Windows, you have to set the vars in UI manually.

The config file gives persistent setup without a hassle and allows easy configuration backups.

That's fair.

Yes, I am asking for more than the initial PR. It would be great if you could continue to work on PR, but I understand if it is too much.

Yeah, I'm not sure I can commit the time to doing much more on this PR than I've already done. Thanks for understanding.

[rocketraman]

@aartiPl Following up on my previous message, is there anything small(ish) I can do on this PR to get it over the finish line?

[aartiPl]

You have already done great work with the initial implementation. I think it is already more than halfway to accomplishing the PR. You can help yourself with the remaining tasks by splitting them into smallish subtasks in such a way that implementation will take you no more than an hour. And first of all: take your time - we all are doing our best, but that's completely normal that there are other priorities in our lives, which often take precedence.