Updates to docs, removing unneeded flags, updating buckets #66
Conversation
aws/tf/sra.tf
Outdated
|
|
||
| // Account - Unity Catalog - Data: | ||
| enable_read_only_external_location = false // Creates a read-only external location and corresponding IAM role, based on the value of the data_bucket variable | ||
| read_only_data_bucket = null // Accepts an S3 path (e.g. "s3://data-bucket") |
There was a problem hiding this comment.
The example in the comment references the bucket URL, but in file uc_external_location.tf, this variable has the s3:// prefix prepended. Just the bucket name works so (e.g. "s3://data-bucket") --> (e.g. "data-bucket")
|
@ryan-gord-db - great catch, updated! |
| @@ -82,6 +81,12 @@ variable "enable_cluster_boolean" { | |||
| sensitive = true | |||
| } | |||
|
|
|||
| variable "enable_read_only_external_location" { | |||
| description = "Flag to enable read only external location" | |||
There was a problem hiding this comment.
You mentioned this is a read only, optional bucket, so shouldn't we add a default value of false here that way you don't have to define this variable to use the module?
There was a problem hiding this comment.
I see its nullable so you can pass in null, but I think that's confusing.
There was a problem hiding this comment.
the bucket is provided by the user, so if they decide to bring their own they can flip it to true
let me remove the not nullable part to avoid any downstream errors, so they're either enabling it or not
There was a problem hiding this comment.
Right, I just mean that the best practice for writing a module being to provide default values for "optional" fields
Right now you have to include a line that sets it to false, which is how we have it structured in the example, but I think best practice is to set default variables here to indicate a true "optional" field
Also to indicate to any readers that that generally we would expect it to be false.
|
@metrocavich - updated in new commit, let me know I took this and applied to all the optional variables of enabling certain features |
|
Left a react but forgot to comment, makes a lot more sense to me, thanks for humoring me. |
|
@metrocavich - let me know if you have anything else or will get it merged |
|
This looks all fantastic to me now. |
This PR address the following issues:
Most notably, the external location and data bucket are no longer required given the fact that catalog isolation has been implemented.