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
Tabulate renaming decisions #649
Conversation
Diff for pulumi-random with merge commit 8e7c881 |
Diff for pulumi-azuread with merge commit 8e7c881 |
Diff for pulumi-gcp with merge commit 8e7c881 |
Diff for pulumi-azure with merge commit 8e7c881 |
Diff for pulumi-aws with merge commit 8e7c881 |
Diff for pulumi-azuread with merge commit 073c431 |
Diff for pulumi-random with merge commit 073c431 |
Diff for pulumi-gcp with merge commit 073c431 |
Diff for pulumi-azure with merge commit 073c431 |
Diff for pulumi-azuread with merge commit 7c67373 |
Diff for pulumi-random with merge commit 7c67373 |
Diff for pulumi-gcp with merge commit 7c67373 |
Diff for pulumi-aws with merge commit 073c431 |
Diff for pulumi-azure with merge commit 7c67373 |
Diff for pulumi-aws with merge commit 7c67373 |
Diff for pulumi-random with merge commit fa41c25 |
Diff for pulumi-azuread with merge commit fa41c25 |
Diff for pulumi-gcp with merge commit fa41c25 |
Diff for pulumi-azure with merge commit fa41c25 |
Diff for pulumi-aws with merge commit fa41c25 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify what consumes this table? It sounds like the table is only consumed by the provider that embeds it.
I'm a little concerned that this will prevent pulumi from round-tripping the schema. I'm not sure that we do, but it seems like a capability we will decide to want.
This should not be part of schema, it's only going to be used by two things. That second part is what would interact with the conversion mapper changes I'm making in pulumi/pulumi#11378, and is a good prompt that we probably need to version the data format there for plugins even though to the engine its just bytes. |
I don't mind not mixing this data into the schema, that is just a technical convenience to achieve the effect of precomputing the data and baking it into the provider for runtime access:
Can totally use another route to get that to happen. Now extending ProviderInfo does not make sense to me at the moment, ProviderInfo seems to be authored by hand or some semi-automated process to configure tfgen. So ProviderInfo is an input to tfgen, while Renames are an output of tfgen. Renames are f(input, tfgen.logic).
The part I want to understand here is versioning/releases. Currently when we release a new version of pulumi-provider-aws say, we get a new schema built and as well as a new provider binary, and we track bw-compatibility there. I'd like to think TF renamings are published and versioned as part of this process. If this is how it's versioned, any tool like tf2pulumi should be able to grab any version of pulumi-provider-aws and respect that version's renaming decisions, even if those are changing between versions. So perhaps Program conversion system should instead query the provider for this? Or how does tf2pulumi read ProviderInfo, does it query the provider for it? |
Yes I'm adding a new GetMapping method to the provider interface for providers to implement so they can send data to tf2pulumi. |
I would like to make use of this on the t0yv0/tfpf-stubs branch right away, with a possible refactor of the current bridge later on. The new features include "structured types like objects". I cannot quite work out how existing code supports inflecting the properties of these at runtime, although tfgen does support a form of object types arising in nested blocks. So for now rewriting how current tfbridge and tf2pulumi inflect names to use this table is out of scope of this PR. Presumably tf2pulumi is naming things right as implemented at the moment. But such a refactor would be directionally aligned with this PR. There's two options:
Here is a demonstration on how this works for resource properties: It's not too bad for resources, but how does it work for named object types (which also may have inflected properties presumably)? I found something like this but it does not seem to be used at the right place?
2 is not immediately beneficial but I think much more fool-proof long term if we can afford storing these tables. |
f8f81a5
to
17ccd48
Compare
a55d89a
to
5c3c037
Compare
Diff for pulumi-random with merge commit 4e02ab5 |
Diff for pulumi-azuread with merge commit 4e02ab5 |
5c3c037
to
70e5c34
Compare
Diff for pulumi-gcp with merge commit 4e02ab5 |
Diff for pulumi-random with merge commit 3319aae |
Diff for pulumi-azuread with merge commit 3319aae |
Diff for pulumi-azure with merge commit 4e02ab5 |
Diff for pulumi-gcp with merge commit 3319aae |
Diff for pulumi-azure with merge commit 3319aae |
Diff for pulumi-aws with merge commit 4e02ab5 |
@@ -0,0 +1,331 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we will have this file now available in the repo and linked into the provider binary? Just a note that we will need to be mindful to support rolling this out across the providers as part of a rollout for tf bridge. Might involve some ci-mgmt changes etc.
}) | ||
} | ||
|
||
// Like Main but allows to customize the generation logic past the parsing of cmd-line arguments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guineveresaenger and @viveklak I had questions about this in a few places, answering here. This annoying looking logic supports a divergent behavior between legacy bridged providers and TFPF providers around generating the renames.json file.
So that:
- legacy bridged providers continue using Main that does not generate renames.json
- whereas TFPF bridged providers use a MainWithCustomGenerate that goes ahead and generates the renames.json
SO the good news is that accepting this PR does not generate any downstream work in the bridged provider boilerplates, but the bad news perhaps is that this does not look particularly elegant or clear; if you have a better idea of where to make the option of generating renames configurable LMK I can clean this up in a follow-up PR.
70e5c34
to
c5fcca4
Compare
Diff for pulumi-random with merge commit 3218a7e |
Diff for pulumi-azuread with merge commit 3218a7e |
Diff for pulumi-gcp with merge commit 3218a7e |
Diff for pulumi-azure with merge commit 3218a7e |
Diff for pulumi-aws with merge commit 3218a7e |
On top of: #661
Introduce Renames type to tabulate name mappings from Pulumi to Terraform.
The motivation for this is translating names in bridged providers at runtime, specifically intended for use with Plugin Framework providers.
The alternative to having this data tabulated is using functions to compute the names:
This functions need data derived from MarshallableProviderInfo metadata passed into their inputs to work correctly. This I think is what happens in today's production providers. These functions also need form a bijection - this holds in practice but it seems like there might be corner cases where the names do not turn around.
Here is why I think explicit rename tables are an improvement (and I'd like to move forward with introducing this and using it for PF):
foolproof to interpret correctly
only TerraformToPulumiName function is eventually needed, PulumiToTerraformName is not needed and does not need to correctly form a bijection; tables are invertible
lets PF bridge to avoids shim dependency at runtime (though this may not work out)
A counter argument on why the tables are detrimental: