You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2023/01/16 12:22:14 UTC
[GitHub] [beam] MakarkinSAkvelon opened a new pull request, #25022: [Playground] Add config.g.dart+Fix gradle
MakarkinSAkvelon opened a new pull request, #25022:
URL: https://github.com/apache/beam/pull/25022
resolves #24200
This PR add config.g.dart file with standard volumes
Change build.gradle.kts file (changing values in config.g.dart file during execution)
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/get-started-contributing/#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] alexeyinkin commented on a diff in pull request #25022: [Playground] Add config.g.dart+Fix gradle
Posted by GitBox <gi...@apache.org>.
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damondouglas commented on a diff in pull request #25022: [Playground] Add config.g.dart+Fix gradle
Posted by GitBox <gi...@apache.org>.
damondouglas commented on code in PR #25022:
URL: https://github.com/apache/beam/pull/25022#discussion_r1072575604
##########
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:
@MakarkinSAkvelon / @alexeyinkin Do we really need a gradle task wrapper to generate the `config.g.dart` file? What would be the consequences if we do not automate its generation?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] alexeyinkin commented on pull request #25022: [Playground] Add config.g.dart+Fix gradle
Posted by "alexeyinkin (via GitHub)" <gi...@apache.org>.
alexeyinkin commented on PR #25022:
URL: https://github.com/apache/beam/pull/25022#issuecomment-1457898454
config.g.dart is deleted here instead:
- https://github.com/apache/beam/pull/25610
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] MakarkinSAkvelon closed pull request #25022: [Playground] Add config.g.dart+Fix gradle
Posted by "MakarkinSAkvelon (via GitHub)" <gi...@apache.org>.
MakarkinSAkvelon closed pull request #25022: [Playground] Add config.g.dart+Fix gradle
URL: https://github.com/apache/beam/pull/25022
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] alexeyinkin commented on a diff in pull request #25022: [Playground] Add config.g.dart+Fix gradle
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #25022:
URL: https://github.com/apache/beam/pull/25022#discussion_r1073260716
##########
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:
I am not an expert in deployment. It `config.g.dart` already generated with Gradle, and this PR just allows this file to pre-exist under some conditions. Given that we are tired of copying `config.example.dart` to `config.g.dart` manually, I think this is worthy short-term.
Mid-term I just want something clean and simple, idempotent with respect to command runs, and that will allow me any changes in the version-controlled copy. This currently is not idempotent and requires us to preserve a specific comment in the version-controlled copy.
Whether we should merge this first or go directly with the mid-term goal is not up to me because I do not know the priorities in DevOps.
I also think we also need a diagram of runs, like what starts what, because I am currently lost in this.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] alexeyinkin commented on a diff in pull request #25022: [Playground] Add config.g.dart+Fix gradle
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #25022:
URL: https://github.com/apache/beam/pull/25022#discussion_r1073260716
##########
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:
I am not an expert in deployment. `config.g.dart` already generated with Gradle, and this PR just allows this file to pre-exist under some conditions. Given that we are tired of copying `config.example.dart` to `config.g.dart` manually, I think this is worthy short-term.
Mid-term I just want something clean and simple, idempotent with respect to command runs, and that will allow me any changes in the version-controlled copy. This currently is not idempotent and requires us to preserve a specific comment in the version-controlled copy.
Whether we should merge this first or go directly with the mid-term goal is not up to me because I do not know the priorities in DevOps.
I also think we also need a diagram of runs, like what starts what, because I am currently lost in this.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] MakarkinSAkvelon commented on a diff in pull request #25022: [Playground] Add config.g.dart+Fix gradle
Posted by "MakarkinSAkvelon (via GitHub)" <gi...@apache.org>.
MakarkinSAkvelon commented on code in PR #25022:
URL: https://github.com/apache/beam/pull/25022#discussion_r1093269220
##########
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:
In case we leave this file - config.g.dart - "as-is" Frontend will use the wrong Backend URLs and the playground will not work at all
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] MakarkinSAkvelon commented on pull request #25022: [Playground] Add config.g.dart+Fix gradle
Posted by GitBox <gi...@apache.org>.
MakarkinSAkvelon commented on PR #25022:
URL: https://github.com/apache/beam/pull/25022#issuecomment-1385303147
R @damondouglas
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org