Move towards consistently specifying the type of a Map in a nested *Schema elemnt

[jimmy-btn]

When specifying the type of a Map in a provider schema, we have two options which are (today) inconsistently applied - although Option 1 is more commonly used.

Option 1: Provide a ValueType directly to the Elem

"field": &schema.Schema{
    Type:         schema.TypeMap,
    Elem:         schema.TypeString,
}

Option 2: Specify the Type using a nested Schema

"field": &schema.Schema{
    Type:         schema.TypeMap,
    Elem:         &schema.Schema{Type:schema.TypeString},
}

Different parts of the helper/schema codebase make different assumptions about which approach will be used which can cause panics. I would like to move towards consistent use of one or the other.

Option 2 is my preferred approach, as it:

• Is consistent with how lists of scalar types are declared (big win!)
• Simplifies possible types for Elem (only *Schema or *Resource)
• Will make it easier if we ever want Maps of complex elements (e.g. a Map of Lists)

My plan is:

1. Merge this PR, which will allow resources to be read/written if a schema uses the notation in Option 2.
2. Wait for major providers (AWS/Google) to vendor this change
3. Update these providers to consistently use Option 2 syntax
4. Update InternalValidate to validate the type of Elem for Maps and Lists
5. Remove the code which supports Option 1

Alternative:

The alternative is to continue to support the existing Option 1 syntax and audit and fix anywhere that we expect Elem to be a *Schema or a *Resource. I'm equally happy with this, although I think it creates an unexpected difference in the way Maps and Lists are specified.

[apparentlymart]

Thanks for looking into this, @jimmy-btn!

This is a timely observation since we're currently laying the groundwork to support TypeMap schemas that have non-string value types, including *schema.Resource value types as we can do for TypeList and TypeSet. The latter would use the familiar block label syntax to provide a key for each nested block:

# NOT YET IMPLEMENTED and may change before release

resource "example" "foo" {
  nested "foo" {
    # ...
  }
  nested "bar" {
    # ...
  }
}

Getting the existing usage of TypeMap straightened out would be a great help when we come to add support for the above, and I agree with your suggestion that we adopt your option 2 here to align with the approach used for TypeList and TypeSet. (Indeed, our work-in-progress code for the above would actually crash if option 1 were in use, so I'm glad you spotted this before we tried to launch that!)

It looks like your goal here with this first PR is to support both forms without crashing. Is that right? If so, it would be great to also have some tests that exercise both cases and show them both working for now, and then we can remove the "option 1" test once we reach your step 5. Do you think you have time to write some tests here? If not, I can take a look at it once I have some time.

Thanks again for working on this!

[jimmy-btn]

Apologies for the slow reply.

Sounds like we're on the same page - I'll add some tests to this PR in the next day or two.

It sounds like getting to Step 3 is going to gate the roll out of your planned changes, so I'll make sure to shepard the rest of this along quickly. The long pole will likely be working out how to best get the larger provides to re-vendor. If there's a standard process for this, please let me know!

[jimmy-btn]

Tests added. I have confirmed that the new tests fail against master (or at least, those tests using Option 2 do), and all pass with my proposed changes.

This gives me confidence that both syntaxes will be supported once this PR lands.

[jimmy-btn]

Checking in - I believe this is ready to merge, but please let me know if you're waiting on anything else from me.

[apparentlymart]

Hi @jimmy-btn! Sorry for the slow responses here... we've been reorganizing our focuses within the team recently and as a result I lost track of this one. 😖

I'd like to check in with my fellow team members who now have the provider SDK as part of their focus area before I move foward here. I've dropped a note, and we'll follow up here soon.

[jimmy-btn]

No problem at all! Here to help if it's useful!

…

On Fri, Mar 9, 2018 at 5:27 PM, Martin Atkins ***@***.***> wrote: Hi @jimmy-btn <https://github.com/jimmy-btn>! Sorry for the slow responses here... we've been reorganizing our focuses within the team recently and as a result I lost track of this one. 😖 I'd like to check in with my fellow team members who now have the provider SDK as part of their focus area before I move foward here. I've dropped a note, and we'll follow up here soon. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17097 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Acsi_RSGjl_fLzs6VUDsnzkI3JXps81Tks5tcwG0gaJpZM4Rcd_F> .

[apparentlymart]

Hi again @jimmy-btn! Sorry for all the back-and-forth here.

We talked this through internally and everyone is happy with the plan you laid out here.

For getting individual providers updated, we usually want to pin the vendoring in a provider to a Terraform release tag and so we'll probably need to hold until the 0.10.4 release is out (planned soon) in order to take the next step here. Once that is out, a PR to each of the relevant providers that just includes the result of updating the vendoring to the 0.10.4 tag would be the best place to start, and assuming the tests still pass after that (and I'd be surprised if they don't) they could then merge other PRs to update to the new standard usage in your option 2. Vendor changes are usually handled separately just because the vendor directory can be quite "conflict-y" and so doing changes in isolation makes them more visible and thus reduces the chances of merge conflicts.

I think before we remove the support for the Option 1 form here we'd prefer to have at least the providers listed under the "Major Cloud" category, since those are the ones most commonly used and where most active development is going on that might otherwise be disrupted by this change.

Thanks again for working on this! From the above discussion it sounded like you were motivated to make these follow-on changes yourself, which would be much appreciated. If you find that you're not able to complete this then that's totally fine and understandable; please just let us know and we can see about completing this work a different way. If our current project to improve the underlying architecture here "catches up" with your work then we should be able to help things along as part of that work.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.