damondouglas commented on a change in pull request #16549:
URL: https://github.com/apache/beam/pull/16549#discussion_r799645175
##########
File path: playground/terraform/README.md
##########
@@ -62,11 +62,13 @@ Do you want to perform these actions?
Enter a value:
```
-Type `yes` and hit **Enter**. Applying the configuration could take several
minutes. When it's finished, you should see `Apply complete!` along with some
other information, including the number of resources created.
+Type `yes` and hit **Enter**. Applying the configuration could take several
minutes. When it's finished, you should
+see `Apply complete!` along with some other information, including the number
of resources created.
### Applying a non-default configuration
-You can apply a non-default Terraform configuration by changing the values in
the `terraform.tfvars` file. The following variables are available:
+You can apply a non-default Terraform configuration by changing the values in
the `terraform.tfvars` file. The following
Review comment:
A `*.tfvars` file is typically in the same directory of the terraform
code. Following up on the previous comment, it isn't clear to the reader where
the tfvars file should be created. I wonder if it makes more sense to not even
document on any terraform workflow and refer to terraform's documentation on
this subject.
##########
File path: playground/terraform/README.md
##########
@@ -29,14 +29,14 @@ You'll also need to make sure that you're currently logged
into your GCP account
```bash
$ gcloud auth login
```
-In other case you'll need an environment variable
`GOOGLE_APPLICATION_CREDENTIALS` set to JSON key for service account that will
be used to deploy resources
+
+In other case you'll need an environment variable
`GOOGLE_APPLICATION_CREDENTIALS` set to JSON key for service account
Review comment:
Should we be recommending the use of a JSON key? Wouldn't a more secure
practice involve the use of service account impersonation?
##########
File path: playground/terraform/README.md
##########
@@ -62,11 +62,13 @@ Do you want to perform these actions?
Enter a value:
```
-Type `yes` and hit **Enter**. Applying the configuration could take several
minutes. When it's finished, you should see `Apply complete!` along with some
other information, including the number of resources created.
+Type `yes` and hit **Enter**. Applying the configuration could take several
minutes. When it's finished, you should
Review comment:
The position of this README in the terraform folder and its description
of how to run the terraform code, implies
a single terraform workflow in this terraform folder. It isn't clear to
someone coming to this repository for the first time that they need to run the
code in each of the folders. Also, it isn't clear if there is an order or
dependency between each of the terraform code folders.
##########
File path: playground/terraform/README.md
##########
@@ -92,10 +94,10 @@ After applying terraform following resources will be
created:
* GCP [Artifact Registry](https://cloud.google.com/artifact-registry) used to
store application docker files:
* VPC to run GCP App Engine VM's ([VPC](https://cloud.google.com/vpc))
-* 2 buckets to store Examples for Playground Application and Terraform states
[buckets](https://cloud.google.com/storage/docs/key-terms#buckets)
+* 2 buckets to store Examples for Playground Application and Terraform
+ states [buckets](https://cloud.google.com/storage/docs/key-terms#buckets)
* 2 GCP [App Engine](https://cloud.google.com/appengine) services for backend
and frontend applications
-
### Destroying your cluster
Review comment:
The use of the word `cluster` is misleading in my opinion as the
terraform code doesn't just create a "cluster". Is it even necessary to
document on a workflow that is already documented by terraform?
##########
File path: playground/terraform/README.md
##########
@@ -62,11 +62,13 @@ Do you want to perform these actions?
Enter a value:
```
-Type `yes` and hit **Enter**. Applying the configuration could take several
minutes. When it's finished, you should see `Apply complete!` along with some
other information, including the number of resources created.
+Type `yes` and hit **Enter**. Applying the configuration could take several
minutes. When it's finished, you should
+see `Apply complete!` along with some other information, including the number
of resources created.
### Applying a non-default configuration
-You can apply a non-default Terraform configuration by changing the values in
the `terraform.tfvars` file. The following variables are available:
+You can apply a non-default Terraform configuration by changing the values in
the `terraform.tfvars` file. The following
+variables are available:
Review comment:
I would like to propose removing this section and rely on terraform
variable description properties. Documenting on variables in a README makes
the terraform codebase not as orthogonal as it could be. I've seen many
situations where there is misalignment between the terraform variables and the
documentation. Instead of listing out the variables and whether they are
required in a README, one could simply provide a description of the variable in
the terraform code directly and when using an IDE such as VS code or IntelliJ,
the IDE is great about parsing these descriptions. If a default value is
provided, the IDE will communicate to the developer whether the variable is
optional. Relying only on terraform variables self documenting provides a
single source of truth and preserves orthogonality of the code.
##########
File path: playground/terraform/applications/backend-go/main.tf
##########
@@ -35,22 +35,22 @@ resource "google_app_engine_flexible_app_version"
"backend_app_go" {
automatic_scaling {
max_total_instances = 7
min_total_instances = 2
- cool_down_period = "120s"
+ cool_down_period = "120s"
cpu_utilization {
target_utilization = 0.7
}
}
resources {
memory_gb = 16
- cpu = 8
+ cpu = 8
}
env_variables = {
- CACHE_TYPE="${var.cache_type}"
- CACHE_ADDRESS="${var.cache_address}:6379"
- NUM_PARALLEL_JOBS=30
- LAUNCH_SITE = "app_engine"
+ CACHE_TYPE = var.cache_type
+ CACHE_ADDRESS = "${var.cache_address}:6379"
Review comment:
Do we want to hard code the port?
##########
File path: playground/terraform/applications/backend-java/main.tf
##########
@@ -44,15 +44,15 @@ resource "google_app_engine_flexible_app_version"
"backend_app" {
}
env_variables = {
- CACHE_TYPE="${var.cache_type}"
- CACHE_ADDRESS="${var.cache_address}:6379"
- NUM_PARALLEL_JOBS=10
- LAUNCH_SITE = "app_engine"
+ CACHE_TYPE = var.cache_type
+ CACHE_ADDRESS = "${var.cache_address}:6379"
Review comment:
Do we want to hard code the port?
--
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]
