alexeyinkin commented on code in PR #25022:
URL: https://github.com/apache/beam/pull/25022#discussion_r1071316149
##########
playground/terraform/build.gradle.kts:
##########
@@ -295,7 +295,25 @@ tasks.register("prepareConfig") {
val configFileName = "config.g.dart"
val modulePath =
project(":playground:frontend").projectDir.absolutePath
var file = File("$modulePath/lib/$configFileName")
-
+ val lines = file.readLines()
+ val endOfSlice = lines.indexOfFirst { it.contains("Generated content
below") }
+ if (endOfSlice != -1) {
+ val oldContent = lines.slice(0 until endOfSlice)
+ val flagDelete = file.delete()
+ if (!flagDelete) {
+ throw kotlin.RuntimeException("Deleting file failed")
+ }
+ val sb = kotlin.text.StringBuilder()
+ val lastLine = oldContent[oldContent.size - 1]
+ oldContent.forEach {
+ if (it == lastLine) {
+ sb.append(it)
+ } else {
+ sb.appendLine(it)
+ }
+ }
+ file.writeText(sb.toString())
+ }
Review Comment:
This Gradle task can be executed in these two cases:
1. The user has just typed in the command to deploy the system for the first
time after checking it out. `config.g.dart` is as the version-controlled copy.
It has the production URLs which developers normally use for their local runs.
They are of no meaning to the deployment and must be overwritten. Here the
string "Generated content below" is the flag that this script uses to detect
that `config.g.dart` has not been written with the deployment URLs. It then
overwrites `config.g.dart` using `dns-name` property value. This property value
must be provided.
2. The user is re-deploying the system from the same checked-out directory,
so `config.g.dart` already contains the deployment URLs. The idea was to allow
the user to re-run the task without specifying `dns-name` to use the same value
as before. Since `config.g.dart` now does not contains "Generated content
below" line, it will not be overwritten. Handling this case is the only purpose
of preserving the content.
If we must accept this fast, then this is the way to go because normally we
will only run this once per check-out-and-deploy cycle. This is my guess
because I am not an expert in Gradle.
But if we have time to clean this up, I suggest doing so, because:
1. Seems like we do not support re-running the task. Since "Generated
content below" will not be found in the overwritten file, `file.delete()` will
not be called, and so `config.g.dart` will be appended with new values, which
in Dart means duplicate definition of constants, so it will not build.
2. If we fix this, the very search for a content in the file is an
unreliable flag for a need to overwrite it because the content may change for
reasons outside of this script. I see the following as more reliable:
- Always overwrite the file if `dns-name` property is given. Always skip any
writes if `dns-name` is not given.
- Always require `dns-name` and throw if it is not provided.
I personally think that we should require all parameters be given when
running the task. I can foresee other configurable options that are
deployment-specific:
- Google Analytics counter ID.
- Debug flags.
While it is easy to add `-Pname=value` flags for them to a Gradle task and
to write them into `config.g.dart`, it is increasingly hard to allow omitting
any of them.
@damondouglas what do you suggest?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
