Fix metadata computation for empty bundle #939
Conversation
| @@ -57,6 +69,6 @@ func ValidateState(state *tfjson.State) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func Load() bundle.Mutator { | |||
| return &load{} | |||
| func Load(modes ...loadMode) bundle.Mutator { | |||
There was a problem hiding this comment.
Are multiple load modes necessary?
There was a problem hiding this comment.
No, infact there probably wont be multiple modes anytime soon. The reason I went with this approach is this is more readable at the call site:
terraform.Load(terraform.NoErrorOnEmptyState)
The alternatives add a boolean field arg (not readable at call site) or a enum for the "goal/mode" of load which also did non seem like a good fit here.
There was a problem hiding this comment.
Oh I see, it's for zero or one. We can change the callsite to be explicit ("ErrorOnEmptyState") or have two public methods, one with one parameter and one with no parameters. What do you think?
There was a problem hiding this comment.
Exactly it's zero to one. I'm against having two methods because there's no need for it, most of the code would be dup.
Between ErrorOnEmptyState and NoErrorOnEmptyState, I am ok with either. I preferNoErrorOnEmptyState here because the branching is clearer in that case. Note, we cannot call TerraformToBundle if the state is empty. .
Would renaming the mode to AllowEmptyState be more readable?
There was a problem hiding this comment.
As a general rule, don't use negatives in constant names because double negatives read odd. I.e. if I write if !NoErrorOnFoo is harder to parse than if ErrorOnFoo.
There was a problem hiding this comment.
You only need two Load methods, essentially two constructors.
func Load() bundle.Mutator { ... } // or LoadDefault()
func LoadAllowEmptyState() bundle.Mutator { ... } // in which case you don't need a separate options enum
// or
func Load(allowEmptyState boolean) // or LoadWithOptionsThe main validation logic could still be the same. This approach just seems unnecessarily generic for what we're trying to do here.
There was a problem hiding this comment.
We follow this pattern of variadic args for 0/1 modes in other places too. For example:
Lines 17 to 21 in d70d744
IMO it's better to expose a single public method here, and allow users to toggle the validation they require using the mode arg. Two public APIs seem unnecessary even if the most the code would be common.
I have refactored the code to use ErrorOnEmptyState instead though now.
There was a problem hiding this comment.
Ok, happy to go with precedent if it exists. I guess this kind of pattern is used for functional options too. Given that this is also quasi-internal to DABs, maybe it is fine.
There was a problem hiding this comment.
I guess I'm a purist, but to me, it doesn't feel good to allow users to call functions in meaningless ways (in this case, with 2 or more values for this option). I'm pro-referential transparency, pro-"expose the exact interface you permit".
| @@ -57,6 +69,6 @@ func ValidateState(state *tfjson.State) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func Load() bundle.Mutator { | |||
| return &load{} | |||
| func Load(modes ...loadMode) bundle.Mutator { | |||
There was a problem hiding this comment.
As a general rule, don't use negatives in constant names because double negatives read odd. I.e. if I write if !NoErrorOnFoo is harder to parse than if ErrorOnFoo.
CLI: * Fix URL for bundle template documentation ([#903](#903)). * Library to convert config.Value to Go struct ([#904](#904)). * Loading an empty file yields a nil ([#906](#906)). * Fix pattern validation for input properties ([#912](#912)). * Simplified code generation logic for handling path and request body parameters and JSON input ([#905](#905)). * Add support for multiline descriptions when using template enums ([#916](#916)). * Move bundle configuration filename code ([#917](#917)). * Add configuration normalization code ([#915](#915)). * Add welcome message to bundle templates ([#907](#907)). * Consolidate bundle configuration loader function ([#918](#918)). * Upload terraform state even if apply fails ([#923](#923)). * Use UserName instead of Id to check if identity used is a service principal ([#924](#924)). * `make snapshot` to build file in `.databricks/databricks` ([#927](#927)). * Persist deployment metadata in WSFS ([#845](#845)). * Run make fmt from fmt job ([#929](#929)). * Add override to support YAML inputs for apps ([#921](#921)). * Add GitHub issue templates ([#925](#925)). * Remove resolution of repo names against the Databricks Github account ([#940](#940)). * Fix metadata computation for empty bundle ([#939](#939)). Bundles: * **FILL THIS IN MANUALLY BY MOVING RELEVANT ITEMS FROM ABOVE LIST** Internal: * **FILL THIS IN MANUALLY BY MOVING RELEVANT ITEMS FROM ABOVE LIST** API Changes: * Added `databricks apps` command group. * Added `databricks account network-policy` command group. OpenAPI commit 5903bb39137fd76ac384b2044e425f9c56840e00 (2023-10-23) Dependency updates: * Bump google.golang.org/grpc from 1.58.2 to 1.58.3 ([#920](#920)). * Bump the Go SDK in the CLI ([#919](#919)). * Bump Terraform provider to v1.29.0 ([#926](#926)). * Bump github.com/google/uuid from 1.3.1 to 1.4.0 ([#932](#932)).
Changes
This PR fixes metadata computation for empty bundle. Before we would error because the
terraform.Load()mutator errors on a empty / no state file.Tests
Failing integration tests now pass.