-
Notifications
You must be signed in to change notification settings - Fork 9.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move towards consistently specifying the type of a Map in a nested *Schema elemnt #17097
Conversation
Thanks for looking into this, @jimmy-btn! This is a timely observation since we're currently laying the groundwork to support
Getting the existing usage of 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! |
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! |
…a Map are supported
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. |
Checking in - I believe this is ready to merge, but please let me know if you're waiting on anything else from me. |
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. |
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>
.
|
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 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. |
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. |
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
Option 2: Specify the Type using a nested Schema
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:
My plan is:
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.