Mildwonkey/protocolv6 #27699
Mildwonkey/protocolv6 #27699
Conversation
This is the first commit for plugin protocol v6. This is currently unused (dead) code; future commits will add the necessary conversion packages, extend configschema, and modify the providers.Interface. The new plugin protocol includes the following changes: - A new field has been added to Attribute: NestedType. This will be the key new feature in plugin protocol v6 - Several massages were renamed for consistency with the verb-noun pattern seen in _most_ messages. - The prepared_config has been removed from PrepareProviderConfig (renamed ValidateProviderConfig), as it has never been used. - The provisioner service has been removed entirely. This has no impact on built-in provisioners. 3rd party provisioners are not supported by the SDK and are not included in this protocol at all.
This commit adds a new field, NestedType, to the Attribute schema, and extends the current Attribute decoderSpec to account for the new type. The codepaths are mostly unused and included in a separate commit to verify that the included changes do not impact any other tests yet.
- rename ProposedNewObject to ProposedNew: Now that there is an actual configschema.Object it will be clearer if the function names match the type the act upon. - extract attribute-handling logic from assertPlanValid and extend A new function, assertPlannedAttrsValid, takes the existing functionality and extends it to validate attributes with NestedTypes. The NestedType-specific handling is in assertPlannedObjectValid, which is very similar to the block-handling logic, except that nulls are a valid plan (an attribute can be null, but not a block).
plugin6 includes a `convert` package to handle conversion between the plugin protocol and configschema, and the GRPCProviderPlugin interface implementation for protocol v6.
Codecov Report
|
|
The new I don't recall where validation for things like computed values set in the configuration happens, but that should be done for nested types as well. |
|
🤦 Your comment triggered some unfortunately and uncomfortable memories of me seeing that and forgetting - I'll go add some tests, then fix them! Thank you ❤️ |
with NestedType objects. There are a handful of mostly cosmetic changes in this PR which likely make the diff awkward to read; I renamed several functions to (hopefully) clarifiy which funcs worked with Blocks vs other types. I also extracted some small code snippets into their own functions for reusability. The code that descends into attributes with NestedTypes is similar to the block-handling code, and differs in all the ways blocks and attributes differ: null is valid for attributes, unlike blocks which can only be present or empty.
plans/objchange/objchange.go
Outdated
| if config.Type().IsTupleType() { | ||
| newV = cty.EmptyTupleVal | ||
| } else { | ||
| newV = cty.ListValEmpty(schema.ImpliedType()) |
There was a problem hiding this comment.
I think this is where we're going to want cty.NullVal(schema.ImpliedType()). I'm not sure offhand how to handle the tuple case, I'll have to run some tests
plans/objchange/objchange.go
Outdated
| } else { | ||
| newV = cty.SetValEmpty(blockType.Block.ImpliedType()) | ||
| newV = cty.MapValEmpty(schema.ImpliedType()) |
There was a problem hiding this comment.
And again here, I think we want a case for a null value rather than an empty value. Just like the tuple case above, I'm not sure what the object case will be, but I'll see what the tests look like.
A handful of bugs popped up while extending the testing in plans/objchange. The main themes were failing to recurse through deeply nested NestedType attributes and improperly building up the ImpliedType. This commit fixes those issues and extends the test coverage to match.
…osedNewNestedType
|
@jbardin addressing those last two comments led me down a delightful rabbit hole of rough edges and bugs in the supporting code in Thank you so much for all the reviews. |
| // Schema is the configuration schema for a Resource or Provider. | ||
| message Schema { | ||
| message Block { | ||
| int64 version = 1; |
There was a problem hiding this comment.
I'm still not super clear why blocks get versions separate from the schema version or when you would want to set it/what you would want to set it to.
There was a problem hiding this comment.
Good question and I also do not know; would you like me to investigate removing this field entirely?
UPDATE: Each resource can have its own schema, and that's where it gets set. (ha i just refactored all this code, you'd think I would have remembered!)
Terraform uses that to determine if the resource' state needs to be updated to match the new schema:
What I don't know is usage statistics.
There was a problem hiding this comment.
@jbardin is disagreeing with me and says everything I thought was looking at the resource-specific Block schema version is actually looking at the overall Schema.Version, so I'm back to 🤷 , sorry
| DynamicValue state = 1; | ||
| repeated Diagnostic diagnostics = 2; | ||
| } | ||
| } |
There was a problem hiding this comment.
I noticed we removed provisioners from the protocol--is that intentional?
|
I'm incredibly late to the party, but so glad this merged. 🎉 Only a couple comment/questions from my glance through it, one about provisioners being removed and one about "version" in blocks. |
|
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. |
ghost
locked as resolved and limited conversation to collaborators
Plugin protocol v6 take 3: Yes, this is my third pass at the same PR.
I've incorporated the previous PR's feedback into new commits in a fresh branch so that the commits are (gasp) useful to review individually again. Thanks to everyone who chimed in, this has all been very helpful.
The only thing I've removed from this PR (besides bugs!) vs the earlier one is the
panicinformat/diffwhen a NestedType is encountered. I promise I have not forgotten that this needs to be done.@paddycarver : I did make the various message name changes we discussed in the protocol this time around, so please do let me know if you see anything still missing.