kscript environment variable substitution for repositories no longer works #402

rocketraman · 2023-06-22T06:13:23Z

In kscript 4.2.2, environment variable substitution for repositories no longer works.

The first problem is that the syntax is no longer {{FOO}} -- since the backend is now kotlin scripting, the syntax is now $FOO. See MavenDependenciesResolver.addRepository.

Once that is fixed, the next problem is that the gradle script that kscript writes and executes for --package contains the literal values $FOO, as createGradleRepositoryCredentials uses the raw values set for user and password rather than the resolved values.

The text was updated successfully, but these errors were encountered:

aartiPl · 2023-06-22T07:55:28Z

Please explain what you mean by "variable substitution for repositories no longer works"? Is it a user code problem or in kscript itself?

rocketraman · 2023-06-22T12:47:08Z

Please explain what you mean by "variable substitution for repositories no longer works"? Is it a user code problem or in kscript itself?

@aartiPl Substitution of environment variables for the username and password components of a repository is a documented feature of kscript. See

kscript/README.adoc

Lines 419 to 422 in 1c97c21

    
           // You can use environment variables for user and password when string surrounded by double {} brackets  
        
           // @file:Repository("my-art", "http://localhost:8081/artifactory/authenticated_repo", user="{{ARTIFACTORY_USER}}", password="{{ARTIFACTORY_PASSWORD}}") 
        
           // will be use 'ARTIFACTORY_USER' and 'ARTIFACTORY_PASSWORD' environment variables 
        
           // if the value doesn't found in the script environment  will fail

.

I've submitted PR #403 to illustrate the two issues I have identified. You will note that both test 4 and test 5 currently fail.

Test 4 is at the moment a documentation problem -- the initial implementation of this in kscript used {{FOO}} syntax. But when the backend was changed to kotlin script, the machinery inside kotlin script uses the $FOO syntax. So simply documenting the change is sufficient I think.

Test 5 illustrates an issue with packaging scripts that use custom auth with env vars. I'm not sure this ever worked.

For test 4, the call to the relevant code is

kscript/src/main/kotlin/io/github/kscripting/kscript/resolver/DependencyResolver.kt

Line 32 in 1c97c21

    
           addRepository(RepositoryCoordinates(it.url), makeExternalDependenciesResolverOptions(options))

.

For test 5, the relevant code is

kscript/src/main/kotlin/io/github/kscripting/kscript/code/GradleTemplates.kt

Lines 109 to 114 in 1c97c21

    
               private fun createGradleRepositoryCredentials(repository: Repository): String { 
        
                   if (repository.user.isNotBlank() && repository.password.isNotBlank()) { 
        
                       return """|credentials { 
        
                                 |    username = "${repository.user}" 
        
                                 |    password = "${repository.password}" 
        
                                 |}""".trimMargin()

.

rocketraman · 2023-06-22T14:30:53Z

I submitted 3 PRs to resolve this. Please review.

rocketraman · 2023-06-22T14:36:42Z

The approach I took was to move kscript in a way compatible with Kotlin scripting. An alternate approach would be to keep the syntax as {{FOO}}, resolve these inside kscript, and pass the Kotlin script backend (and Gradle) the resolved vars. This would actually simplify the code a bit, but personally I think kscript moving towards syntactic compatibility with Kotlin Script is a better option, as hopefully the kscript wrapper will get smaller and smaller over time. However, that does make it a breaking change for existing scripts.

aartiPl · 2023-06-22T17:30:01Z

Ok, I see. Your approach to changing templates from {{}} to '$' makes sense to me. The only drawback I see now is that IntelliJ will try to resolve a local variable when it finds '$'. So in the code, it will be necessary to escape '$' with a backslash, or IntelliJ will complain about the non-existing variable. Have you tried it? How does it work? Should we escape '$' or not?

Regarding the implementation. There is already a code resolving username and password. It is in SectionResolver:

The resolution of the URL, username, and password should happen there. I think that the best algorithm would be (for all three parameters):

Check if it starts with the dollar sign, and in such a case, try to replace it with the env variable value
If the env variable is unresolved, and the name is one of the KSCRIPT_REPOSITORY_* names, then we should use those names as they are now (details can still be written in the configuration file)
If variables are still not resolved, we should probably throw an exception

Please merge all 3 PRs into only one; it's much easier for me to test and review. Also, please write at least a simple Unit Test.

And thanks for your great work! 👍

rocketraman · 2023-06-22T20:26:39Z

Ok, I see. Your approach to changing templates from {{}} to '$' makes sense to me. The only drawback I see now is that IntelliJ will try to resolve a local variable when it finds '$'. So in the code, it will be necessary to escape '$' with a backslash, or IntelliJ will complain about the non-existing variable. Have you tried it? How does it work? Should we escape '$' or not?

Kotlin script appears to require it be unescaped i.e. it reads it as a literal value. Adding the escape bypasses Kotlin script's var resolution. While we will now do the parsing for this before it ever gets to Kotlin Script and could change this behavior, I suggest we stay compatible to the syntax used in Kotlin Script. IDEA will likely be updated at some point to correctly interpret the $ in scripts.

Regarding the implementation. There is already a code resolving username and password. It is in SectionResolver:

Ok.

2. If the env variable is unresolved, and the name is one of the KSCRIPT_REPOSITORY_* names, then we should use those names as they are now (details can still be written in the configuration file)

One tweak to this which will be more powerful as it allows environment variables to compose from properties, and also easier to implement:

2. After env variable resolution, if the name is one of the KSCRIPT_REPOSITORY_* names, then we should use those names as they are now

Also, please write at least a simple Unit Test.

Done.

PR submitted for all these changes #406.

rocketraman mentioned this issue Jun 22, 2023

Fix and expand manual tests with auth repo #403

Closed

This was referenced Jun 22, 2023

Fix documentation for environment variable substitution #404

Closed

Fix packaging when using repository authentication with env var substitution #405

Closed

rocketraman linked a pull request Jun 22, 2023 that will close this issue

Fix and improve environment variable resolution in repositories #406

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kscript environment variable substitution for repositories no longer works #402

kscript environment variable substitution for repositories no longer works #402

rocketraman commented Jun 22, 2023

aartiPl commented Jun 22, 2023

rocketraman commented Jun 22, 2023 •

edited

rocketraman commented Jun 22, 2023 •

edited

rocketraman commented Jun 22, 2023 •

edited

aartiPl commented Jun 22, 2023

rocketraman commented Jun 22, 2023 •

edited

kscript environment variable substitution for repositories no longer works #402

kscript environment variable substitution for repositories no longer works #402

Comments

rocketraman commented Jun 22, 2023

aartiPl commented Jun 22, 2023

rocketraman commented Jun 22, 2023 • edited

rocketraman commented Jun 22, 2023 • edited

rocketraman commented Jun 22, 2023 • edited

aartiPl commented Jun 22, 2023

rocketraman commented Jun 22, 2023 • edited

rocketraman commented Jun 22, 2023 •

edited

rocketraman commented Jun 22, 2023 •

edited

rocketraman commented Jun 22, 2023 •

edited

rocketraman commented Jun 22, 2023 •

edited