Platform default: autodetect #30
Conversation
…sing a null_resource themselves
Initial module addition and tests
…ake target to upgrade it with latest
feat: add variables for dependency ordering feat: add flag to disable upgrades and make each instance separate
…est/patch-gcloud-version Update GCloud SDK
* updated README.md [ci skip] * updated CHANGELOG.md [ci skip] * updated README.md [ci skip]
Co-authored-by: Miles Matthias <[email protected]>
| description = "Platform CLI will run on. Defaults to linux. Valid values: linux, darwin" | ||
| default = "linux" | ||
| description = "Platform CLI will run on. Defaults to \"\" (autodetect). Valid values: linux, darwin, \"\"" | ||
| default = "" |
There was a problem hiding this comment.
Can we use a default of null (and also explicitly set the type to string)?
| @@ -67,10 +111,15 @@ resource "null_resource" "copy" { | |||
|
|
|||
| provisioner "local-exec" { | |||
| when = create | |||
| command = local.copy_command | |||
| command = "cp -R ${path.module}/cache/${var.platform == "" ? (local.dynamic_platform_file ? data.local_file.platform_detected[0].content : file(local.platform_file)) : var.platform}/. ${local.cache_path}" | |||
There was a problem hiding this comment.
Can we keep this as a local at the top (for consistency) and possibly split it into two expressions?
| ] | ||
| } | ||
|
|
||
| data "local_file" "platform_detected" { |
There was a problem hiding this comment.
Could we add a brief explanation (in a comment) of how this flow is meant to work?
My understanding:
- On first run,
fileexistswill be false so the flow isnull_resource.platform_detectthen call this data source. - On subsequent runs,
fileexistswill be true so the flow is to directly skip tocopyand usefile(to pick up the flow.
This works, but I wonder if it could be simpler. In particular, I'm not sure there's much value in caching the platform detection since gcloud scripts should generally only need to be invoked ~once in a particular module. So maybe we could always redetect when we need to run the command?
There was a problem hiding this comment.
Your understanding is correct.
The reason I ended up doing it like that is because
- I need a bash script to detect the OS
- The only way I found to get back the value is through a file
- local_file is "dynamically resolved" and can be sequenced with a depends_on, but was causing the plan to always contain an action (dynamic reading of the file each time).
- fileexists is evaluated at the "planing" time so it's not useable during the first run (the file is not created at plan time), but is useable afterward without generating a plan action.
I'll give it another try to use local_file, without generating an action for each run.
I probably missed something in the triggers
|
@bdelv Would you be able to rebase and address review comments? |
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days |
morgante
added
enhancement
ran the tests under Linux and MacOS
updated the docs, and ran the lints
Fixes #26