Add documentation and machine type variables for gcp. #457
Conversation
adam-singer
force-pushed
the
adams/update-gcp-default-instance-size
branch
from
27cce91
to
abee88f
Compare
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @adam-singer)
deployment-examples/terraform/GCP/README.md line 30 at r1 (raw file):
After this is applied goto the [Cloud DNS settings](https://console.cloud.google.com/net-services/dns/zones),
Instead we should give a gcloud command to get these or a terraform command to get these values. I think it'd be easier to follow.
deployment-examples/terraform/GCP/README.md line 48 at r1 (raw file):
REGION=us-central1 ZONE=us-central1-a PREFIX=exdev
nit: I'd remove this, they don't need to know this by default.
deployment-examples/terraform/GCP/README.md line 72 at r1 (raw file):
```sh PROJECT_ID=example-sandbox REGION=us-central1
hmmm, I think we can now extract the zone and region values from the global config. Might be worth doing before publishing these docs?
deployment-examples/terraform/GCP/README.md line 74 at r1 (raw file):
REGION=us-central1 ZONE=us-central1-a PREFIX=exdev
nit: ditto
deployment-examples/terraform/GCP/README.md line 85 at r1 (raw file):
A complete and successful deployment should be able to run remote execution commands from bazel (or other supported build systems).
should also say something like:
... after a few mins for the DNS records and load balancers to pickup the new resources (usually <10 mins)
deployment-examples/terraform/GCP/README.md line 105 at r1 (raw file):
SERVICE_ACCOUNT=123-compute@developer.gserviceaccount.com OS_IMAGE=projects/ubuntu-os-cloud/global/images/ubuntu-2204-jammy-v20231201 DISK=projects/example-sandbox/zones/us-central1-a/diskTypes/pd-standard
nit:
DISK=projects/example-sandbox/zones/$ZONE/diskTypes/pd-standard
deployment-examples/terraform/GCP/README.md line 142 at r1 (raw file):
DNS_ZONE=example-sandbox.example.com CAS="cas.${DNS_ZONE}"
nit: Variable never set.
deployment-examples/terraform/GCP/README.md line 143 at r1 (raw file):
DNS_ZONE=example-sandbox.example.com CAS="cas.${DNS_ZONE}" EXECUTOR="scheduler.${DNS_ZONE}"
nit: ditto.
deployment-examples/terraform/GCP/README.md line 168 at r1 (raw file):
(or any other cloned project). ## Notes
nit: lets remove this section.
deployment-examples/terraform/GCP/README.md line 182 at r1 (raw file):
If things need to be torn down due to misconfiguration or fumbles, leveraging the `project_prefix` will allow scoping of the resource names in such a way
project_prefix is also used so users can launch multiple deployments in the same project. (like two devs working at the same time).
There was a problem hiding this comment.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @allada)
deployment-examples/terraform/GCP/README.md line 30 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
Instead we should give a
gcloudcommand to get these or a terraform command to get these values. I think it'd be easier to follow.
We could do either commands, going to go with the last terrraform example since its less verbose but not super scriptable. Oddly enough state show doesn't serialize into json.
# Print name servers with gcloud
gcloud dns record-sets describe $DNS_ZONE --type=NS --zone=${$PREFIX}-dns-zone
# Print name servers with terraform
terraform show -json | jq '.values.root_module.child_modules[].resources[] | select(.address=="module.native_link.data.google_dns_managed_zone.dns_zone") | .values.name_servers'
terraform state show module.native_link.data.google_dns_managed_zone.dns_zonedeployment-examples/terraform/GCP/README.md line 72 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
hmmm, I think we can now extract the
zoneandregionvalues from theglobalconfig. Might be worth doing before publishing these docs?
I'd need to read some more docs on terraform, mostly just followed what existed, variables.tf defined in module, global and dev, all defining the same information. I would assume we could DRY those up a bit. If we remove the mention of zone and region I would be comfortable doing that on a second pr, terraform is still new to me.
I'd also like to remove the gcloud command that explains launching a "workspace" as it feels bit one off, replace it with a different terraform plan.
deployment-examples/terraform/GCP/README.md line 142 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: Variable never set.
Not sure what you mean, it is set in the line above
$ DNS_ZONE=example-sandbox.example.com
$ CAS="cas.${DNS_ZONE}"
$ EXECUTOR="scheduler.${DNS_ZONE}"
$ echo $CAS $EXECUTOR
cas.example-sandbox.example.com scheduler.example-sandbox.example.comdeployment-examples/terraform/GCP/README.md line 182 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
project_prefixis also used so users can launch multiple deployments in the same project. (like two devs working at the same time).
This is a good point, I'd like to lift that reasoning higher up. I didn't want to explicitly document all parameters or at least not yet in a readme. Might reconsider to get started quickly it can be some overhead to parse terraform scripts directly.
adam-singer
force-pushed
the
adams/update-gcp-default-instance-size
branch
from
abee88f
to
89f36f6
Compare
There was a problem hiding this comment.
Reviewable status: 6 of 10 files reviewed, 4 unresolved discussions (waiting on @allada)
deployment-examples/terraform/GCP/README.md line 30 at r1 (raw file):
Previously, adam-singer (Adam Singer) wrote…
We could do either commands, going to go with the last terrraform example since its less verbose but not super scriptable. Oddly enough
state showdoesn't serialize into json.# Print name servers with gcloud gcloud dns record-sets describe $DNS_ZONE --type=NS --zone=${$PREFIX}-dns-zone # Print name servers with terraform terraform show -json | jq '.values.root_module.child_modules[].resources[] | select(.address=="module.native_link.data.google_dns_managed_zone.dns_zone") | .values.name_servers' terraform state show module.native_link.data.google_dns_managed_zone.dns_zone
Done.
deployment-examples/terraform/GCP/README.md line 48 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: I'd remove this, they don't need to know this by default.
Done.
deployment-examples/terraform/GCP/README.md line 74 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: ditto
Done.
deployment-examples/terraform/GCP/README.md line 85 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
should also say something like:
... after a few mins for the DNS records and load balancers to pickup the new resources (usually <10 mins)
Done.
deployment-examples/terraform/GCP/README.md line 105 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit:
DISK=projects/example-sandbox/zones/$ZONE/diskTypes/pd-standard
Done.
deployment-examples/terraform/GCP/README.md line 142 at r1 (raw file):
Previously, adam-singer (Adam Singer) wrote…
Not sure what you mean, it is set in the line above
$ DNS_ZONE=example-sandbox.example.com $ CAS="cas.${DNS_ZONE}" $ EXECUTOR="scheduler.${DNS_ZONE}" $ echo $CAS $EXECUTOR cas.example-sandbox.example.com scheduler.example-sandbox.example.com
Done.
deployment-examples/terraform/GCP/README.md line 143 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: ditto.
Done.
deployment-examples/terraform/GCP/README.md line 168 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: lets remove this section.
Done.
deployment-examples/terraform/GCP/README.md line 182 at r1 (raw file):
Previously, adam-singer (Adam Singer) wrote…
This is a good point, I'd like to lift that reasoning higher up. I didn't want to explicitly document all parameters or at least not yet in a readme. Might reconsider to get started quickly it can be some overhead to parse terraform scripts directly.
Done.
There was a problem hiding this comment.
Reviewable status: 6 of 10 files reviewed, 2 unresolved discussions
deployment-examples/terraform/GCP/README.md line 48 at r1 (raw file):
Previously, adam-singer (Adam Singer) wrote…
Done.
nit: Most of these are no longer needed.
adam-singer
force-pushed
the
adams/update-gcp-default-instance-size
branch
from
89f36f6
to
bdc2dc5
Compare
There was a problem hiding this comment.
Reviewable status: 6 of 10 files reviewed, 2 unresolved discussions (waiting on @allada)
deployment-examples/terraform/GCP/README.md line 48 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: Most of these are no longer needed.
Done.
adam-singer
force-pushed
the
adams/update-gcp-default-instance-size
branch
from
bdc2dc5
to
9a0e4d7
Compare
There was a problem hiding this comment.
Reviewed 3 of 4 files at r2, all commit messages.
Reviewable status: 9 of 10 files reviewed, all discussions resolved (waiting on @adam-singer)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @adam-singer)
There was a problem hiding this comment.
Reviewed 6 of 9 files at r1, 3 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status:
Description
Type of change
How Has This Been Tested?
terraform validate&&terraform planChecklist
git amendsee some docsThis change is