Skip to content
This repository has been archived by the owner on Oct 6, 2020. It is now read-only.

Read source resources correctly #16

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wookasz
Copy link

@wookasz wookasz commented Dec 21, 2019

What

  • Use correct types for values passed to Set

Why
The package that terraform uses github.com/mitchellh/mapstructure to decode values, does not support sql.NullString.

All the calls to d.Set were returning errors. For whatever reason, ignoring errors is common practice throughout various terraform providers so we're not changing that here.

This change passes the correct primitives and in our cases, none of the values can be null in the database so there is no need for sql.NullString.

Testing

  • Create a warehouse, db, and schema with terraform
  • Delete schema manually, ensure terraform recreates it
  • Delete db manually, make sure terraform recreates it and schema
  • Delete warehouse manually, make sure terraform recreates it
  • Change warehouse size, max cluster size, auto suspend period, and comment manually, make sure terraform sees change and can update

Lukasz Lempart added 4 commits December 20, 2019 13:49
**What**
- Use correct types for values passed to `Set`

**Why**
The package that terraform uses `github.com/mitchellh/mapstructure` to decode values, does not support `sql.NullString`.

All the calls to `d.Set` were returning errors. For whatever reason, ignoring errors is common practice throughout various terraform providers so we're not changing that here.

This change passes the correct primitives and in our cases, none of the values can be null in the database so there is no need for `sql.NullString`.
@wookasz wookasz requested a review from malexovi December 21, 2019 00:29
@@ -182,35 +164,14 @@ func readWarehouse(d *schema.ResourceData, meta interface{}) error {
return fmt.Errorf("Error during show create warehouse: %s", err)
}

//Properties
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this dropping the ability to configure these properties on a warehouse? If so, why?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not. It is cleaning up values that were unused but enumerated here for whatever reason. The original author, shoprunner, just dumped everything in here and actually specified some properties in the schema that are not actually set on the warehouse

The only props supported are enumerated here

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants