Skip to content
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

Detailed TF schema dump #2125

Closed
wants to merge 12 commits into from
Closed

Detailed TF schema dump #2125

wants to merge 12 commits into from

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Jun 25, 2024

Quick implementation for #1982 - seems to work fine.
Works only for sdkv2 providers and the first sdkv2 provider in a muxed provider.

Example with cloudflare: https://gist.github.com/VenelinMartinov/fb79846afeecad883999db007381da8f

I also added a script for splitting the data into rows, example output here: https://gist.github.com/VenelinMartinov/4f29ac159a4d5974faafdea5b3a4420d

fixes #1982

@VenelinMartinov VenelinMartinov changed the title Spew tf schema Detailed TF schema dump Jun 25, 2024
@VenelinMartinov VenelinMartinov requested a review from t0yv0 June 25, 2024 13:21
Copy link

codecov bot commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 59.52381% with 51 lines in your changes missing coverage. Please review.

Project coverage is 60.63%. Comparing base (0b6ff1b) to head (7eb64d8).
Report is 107 commits behind head on master.

Files Patch % Lines
pf/tfbridge/main.go 0.00% 16 Missing ⚠️
pf/internal/muxer/muxer.go 0.00% 10 Missing ⚠️
pkg/tfshim/sdk-v2/provider.go 92.59% 6 Missing ⚠️
pkg/tfbridge/main.go 0.00% 5 Missing ⚠️
pf/internal/schemashim/provider.go 0.00% 2 Missing ⚠️
pf/proto/protov6.go 0.00% 2 Missing ⚠️
pkg/tfshim/schema/provider.go 0.00% 2 Missing ⚠️
pkg/tfshim/sdk-v1/provider.go 0.00% 2 Missing ⚠️
pkg/tfshim/tfplugin5/provider.go 0.00% 2 Missing ⚠️
pkg/tfshim/util/filter.go 0.00% 2 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2125      +/-   ##
==========================================
- Coverage   60.70%   60.63%   -0.07%     
==========================================
  Files         350      350              
  Lines       45735    45919     +184     
==========================================
+ Hits        27763    27843      +80     
- Misses      16454    16544      +90     
- Partials     1518     1532      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@VenelinMartinov VenelinMartinov marked this pull request as ready for review June 25, 2024 14:30
@VenelinMartinov
Copy link
Contributor Author

We should really add the pulumi <-> TF mappings either here or as a separate file.

Should we also do some transformation to make this into tabular form? It could also be a separate script in order to not complicate the bridge code.

@@ -253,4 +265,8 @@ func (p *simpleSchemaProvider) DataSourcesMap() shim.ResourceMap {
return p.dataSources
}

func (p *simpleSchemaProvider) DetailedSchemaDump() []byte {
panic("Unsupported")
Copy link
Member

Choose a reason for hiding this comment

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

Instead of panicking, can we just return nil. That way we can leave actual panics uncaught.

Suggested change
panic("Unsupported")
return nil

Comment on lines +63 to +67
flags.SetOutput(io.Discard)

err := flags.Parse(os.Args[1:])
contract.IgnoreError(err)

Copy link
Member

Choose a reason for hiding this comment

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

Why does PF need this ceremony when pkg can just call flags.Bool?

	dumpSchema := flags.Bool("get-schema", false, "dump provider schema as JSON to stdout")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

handleFlags takes a MarshallableProviderInfo, which does not have the required fields for DetailedSchemaDump

@@ -244,6 +244,7 @@ type Provider interface {

// Checks if a value is representing a Set, and unpacks its elements on success.
IsSet(ctx context.Context, v interface{}) ([]interface{}, bool)
DetailedSchemaDump() []byte
Copy link
Member

Choose a reason for hiding this comment

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

Let's make sure this has an explanation (and that we give it a context.Contex and an error channel. I would like us to avoid communicating errors with panic.

Suggested change
DetailedSchemaDump() []byte
// DetailedSchemaDump returns output useful for provider authors to diagnose
// bugs in the bridge. It does not have a stable format and should not be
// parsed.
//
// If a provider does not support dumping it's raw schema, it should return
// (nil, nil).
DetailedSchemaDump(context.Context) ([]byte, error)

prov := NewProvider(&schema.Provider{
ResourcesMap: map[string]*schema.Resource{
"test_resource": {
Schema: map[string]*schema.Schema{
Copy link
Member

Choose a reason for hiding this comment

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

We should probably have a test for blocks (with nesting levels) and for nested types.

pkg/tfshim/sdk-v2/provider.go Show resolved Hide resolved
@@ -222,3 +224,149 @@ func (p v2Provider) IsSet(_ context.Context, v interface{}) ([]interface{}, bool
}
return nil, false
}

type tfSchemaMarshaller struct {
Copy link
Member

Choose a reason for hiding this comment

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

Separate files please, not provider.go

"test_resource": {
"Schema": {
"bar": {
"Type": 2,
Copy link
Member

Choose a reason for hiding this comment

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

I'd go ahead and make the enums surface as strings.

@t0yv0
Copy link
Member

t0yv0 commented Jun 26, 2024

This is promising. Maybe emit CSV file(s) straight from Go without the Py wrapper? It's about the same amount of work it seems.

Does this have to be written against sdk-v2 module, is the information exported through shim.Schema not sufficient? Could we make some extensions to that perhaps? So as to cover PF data as well, via the shim layer?

It's more work but I'd really like also to have data describing Pulumi Package schema in a way that can be joined on (or pre-joined) to the data describing the TF provider, so we can query and find resource properties with their TF and Pulumi names side-by-side, etc. Is that possible already?

@VenelinMartinov
Copy link
Contributor Author

Does this have to be written against sdk-v2 module, is the information exported through shim.Schema not sufficient? Could we make some extensions to that perhaps? So as to cover PF data as well, via the shim layer?

A lot of the information we need is sdkv2 specific, like ConflictsWith, ExactlyOneOf, ConfigMode etc - I think that makes sense conceptually too - the stuff they removed for PF is the stuff which causes trouble - I think we need that information to make this really useful so we'd need separate formats for sdkv2 and pf.

It's more work but I'd really like also to have data describing Pulumi Package schema in a way that can be joined on (or pre-joined) to the data describing the TF provider, so we can query and find resource properties with their TF and Pulumi names side-by-side, etc. Is that possible already?

I believe so - I need to add the resource name to each property/nested resource and we'll get the tf <-> pulumi mappings from the pre-existing --get-info.

@t0yv0
Copy link
Member

t0yv0 commented Jun 27, 2024

Expose shim.ExtraData() map[string]string for these?

Unfortunately this feature is not very interesting if it cannot handle PF resources.

@VenelinMartinov
Copy link
Contributor Author

closing in favour of #2315

@mjeffryes mjeffryes added this to the 0.109 milestone Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a way to dump the TF schema of a bridged provider
4 participants