Allow blob, tilde and env vars in app paths #386
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| package ftl.args | ||
|
|
||
| import ftl.util.Utils.fatalError | ||
| import java.io.IOException | ||
| import java.nio.file.FileSystems | ||
| import java.nio.file.FileVisitOption | ||
| import java.nio.file.FileVisitResult | ||
| import java.nio.file.Files | ||
| import java.nio.file.LinkOption | ||
| import java.nio.file.Path | ||
| import java.nio.file.Paths | ||
| import java.nio.file.SimpleFileVisitor | ||
| import java.nio.file.attribute.BasicFileAttributes | ||
| import java.util.EnumSet | ||
|
|
||
| class ArgsFileVisitor(glob: String) : SimpleFileVisitor<Path>() { | ||
| private val pathMatcher = FileSystems.getDefault().getPathMatcher(glob) | ||
| private val result: MutableList<Path> = mutableListOf() | ||
|
|
||
| @Throws(IOException::class) | ||
| override fun visitFile(path: Path, attrs: BasicFileAttributes): FileVisitResult { | ||
| if (pathMatcher.matches(path)) { | ||
| result.add(path) | ||
| } | ||
| return FileVisitResult.CONTINUE | ||
| } | ||
|
|
||
| override fun visitFileFailed(file: Path?, exc: IOException?): FileVisitResult { | ||
| fatalError("Failed to visit $file $exc") | ||
| return FileVisitResult.CONTINUE | ||
| } | ||
|
|
||
| companion object { | ||
| private const val RECURSE = "/**" | ||
| } | ||
|
|
||
| @Throws(java.nio.file.NoSuchFileException::class) | ||
| fun walk(searchPath: Path): List<Path> { | ||
| val searchString = searchPath.toString() | ||
| // /Users/tmp/code/flank/test_app/** => /Users/tmp/code/flank/test_app/ | ||
| val beforeGlob = Paths.get(searchString.substringBefore(RECURSE)) | ||
| // must not follow links when resolving paths or /tmp turns into /private/tmp | ||
| val realPath = beforeGlob.toRealPath(LinkOption.NOFOLLOW_LINKS) | ||
|
|
||
| val searchDepth = if (searchString.contains(RECURSE)) { | ||
| Integer.MAX_VALUE | ||
| } else { | ||
| 1 | ||
| } | ||
|
|
||
| Files.walkFileTree(realPath, EnumSet.of(FileVisitOption.FOLLOW_LINKS), searchDepth, this) | ||
| return this.result | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,9 @@ import java.io.File | |
| import java.math.RoundingMode | ||
| import java.net.URI | ||
| import java.nio.file.Files | ||
| import java.nio.file.Path | ||
| import java.nio.file.Paths | ||
| import java.util.regex.Pattern | ||
|
|
||
| object ArgsHelper { | ||
|
|
||
|
|
@@ -45,6 +48,24 @@ object ArgsHelper { | |
| } | ||
| } | ||
|
|
||
| fun evaluateFilePath(filePath: String): String { | ||
| var file = filePath.trim().replaceFirst("~", System.getProperty("user.home")) | ||
|
There was a problem hiding this comment. ~ should be replaced only if it's the first character in the string the regex should be
|
||
| file = substituteEnvVars(file) | ||
| // avoid File(..).canonicalPath since that will resolve symlinks | ||
| file = Paths.get(file).toAbsolutePath().normalize().toString() | ||
|
|
||
| // Avoid walking the folder's parent dir if we know it exists already. | ||
| if (File(file).exists()) return file | ||
|
There was a problem hiding this comment. I would prefer if we also printed this value to console. Something like - There was a problem hiding this comment. I think it should be printed already when the args are printed to stdout at the start of a test run. |
||
|
|
||
| val filePaths = walkFileTree(file) | ||
| if (filePaths.size > 1) { | ||
| Utils.fatalError("'$file' ($filePath) matches multiple files: $filePaths") | ||
| } else if (filePaths.isEmpty()) { | ||
| Utils.fatalError("'$file' not found ($filePath)") | ||
| } | ||
| return filePaths[0].toAbsolutePath().toString() | ||
| } | ||
|
|
||
| fun assertGcsFileExists(uri: String) { | ||
| if (!uri.startsWith(GCS_PREFIX)) { | ||
| throw IllegalArgumentException("must start with $GCS_PREFIX uri: $uri") | ||
|
|
@@ -135,7 +156,8 @@ object ArgsHelper { | |
| return JsonObjectParser(JSON_FACTORY).parseAndClose( | ||
| Files.newInputStream(defaultCredentialPath), | ||
| Charsets.UTF_8, | ||
| GenericJson::class.java | ||
| )["project_id"] as String | ||
| } catch (e: Exception) { | ||
| println("Parsing $defaultCredentialPath failed:") | ||
| println(e.printStackTrace()) | ||
|
|
@@ -150,4 +172,24 @@ object ArgsHelper { | |
| // Allow users control over projectId by checking using Google's logic first before falling back to JSON. | ||
| return ServiceOptions.getDefaultProjectId() ?: serviceAccountProjectId() | ||
| } | ||
|
|
||
| // https://stackoverflow.com/a/2821201/2450315 | ||
| private val envRegex = Pattern.compile("\\$([a-zA-Z_]+[a-zA-Z0-9_]*)") | ||
| private fun substituteEnvVars(text: String): String { | ||
|
There was a problem hiding this comment. we should check this works for duplicate environment variables via more JUnit tests. There are probably other edge cases as well. For example the
There was a problem hiding this comment. We should probably WARN but not fail for duplicate env variables. The warning is to notify the user that he's duplicated an env variable. Not failing, in case its intentional. Regardless, I don't think we should handle custom env vars. That would likely be a pain to maintain. There was a problem hiding this comment. bash doesn't warn on duplicates. I'm using bash as the model for the parsing logic. |
||
| val buffer = StringBuffer() | ||
| val matcher = envRegex.matcher(text) | ||
| while (matcher.find()) { | ||
| val envName = matcher.group(1) | ||
| val envValue = System.getenv(envName) ?: "" | ||
|
There was a problem hiding this comment. Should we raise an error/warning here? Silently ignoring un-found env variables seems like a bug. There was a problem hiding this comment. this is standard behavior in Bash, that's how environment variables work sadly. There was a problem hiding this comment. discussed on slack, printing a warning makes sense. |
||
| matcher.appendReplacement(buffer, envValue) | ||
| } | ||
| matcher.appendTail(buffer) | ||
| return buffer.toString() | ||
| } | ||
|
|
||
| private fun walkFileTree(filePath: String): List<Path> { | ||
| val searchDir = Paths.get(filePath).parent | ||
|
|
||
| return ArgsFileVisitor("glob:$filePath").walk(searchDir) | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| package ftl.args | ||
|
|
||
| import com.google.common.truth.Truth | ||
| import ftl.test.util.FlankTestRunner | ||
| import ftl.test.util.TestHelper.absolutePath | ||
| import org.junit.Rule | ||
| import org.junit.Test | ||
| import org.junit.contrib.java.lang.system.EnvironmentVariables | ||
| import org.junit.runner.RunWith | ||
| import java.io.File | ||
|
|
||
| @RunWith(FlankTestRunner::class) | ||
| class ArgsHelperFilePathTest { | ||
|
|
||
| @get:Rule | ||
| val environmentVariables = EnvironmentVariables() | ||
|
|
||
| @Test | ||
| fun evaluateBlobInFilePath() { | ||
| val testApkBlobPath = "../test_app/**/app-debug-*.apk" | ||
| val actual = ArgsHelper.evaluateFilePath(testApkBlobPath) | ||
|
|
||
| val testApkRelativePath = "../test_app/apks/app-debug-androidTest.apk" | ||
| val expected = testApkRelativePath.absolutePath() | ||
|
|
||
| Truth.assertThat(actual).isEqualTo(expected) | ||
| } | ||
|
|
||
| private fun makeTmpFile(filePath: String): String { | ||
| val file = File(filePath) | ||
| file.parentFile.mkdirs() | ||
|
|
||
| file.apply { | ||
| createNewFile() | ||
| deleteOnExit() | ||
| } | ||
|
|
||
| return file.absolutePath | ||
| } | ||
|
|
||
| @Test | ||
| fun evaluateTildeInFilePath() { | ||
| val expected = makeTmpFile("/tmp/random.xctestrun") | ||
|
|
||
| val inputPath = "~/../../tmp/random.xctestrun" | ||
| val actual = ArgsHelper.evaluateFilePath(inputPath) | ||
|
|
||
| Truth.assertThat(actual).isEqualTo(expected) | ||
| } | ||
|
|
||
| @Test | ||
| fun evaluateEnvVarInFilePath() { | ||
| environmentVariables.set("TEST_APK_DIR", "test_app/apks") | ||
| val testApkPath = "../\$TEST_APK_DIR/app-debug-androidTest.apk" | ||
| val actual = ArgsHelper.evaluateFilePath(testApkPath) | ||
|
|
||
| val expected = "../test_app/apks/app-debug-androidTest.apk".absolutePath() | ||
|
|
||
| Truth.assertThat(actual).isEqualTo(expected) | ||
| } | ||
|
|
||
| @Test(expected = java.nio.file.NoSuchFileException::class) | ||
| fun evaluateInvalidFilePath() { | ||
| val testApkPath = "~/flank_test_app/invalid_path/app-debug-*.xctestrun" | ||
| ArgsHelper.evaluateFilePath(testApkPath) | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably need more testing around paths with
/**/and/*/i'm not sure splitting by
/**/works for all cases