Add Parameterize to generate an SDK for a specific API version#4010
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
thomas11
force-pushed
the
tkappler/parametrize
branch
from
4557fdc to
54bf233
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4010 +/- ##
==========================================
+ Coverage 57.67% 57.84% +0.16%
==========================================
Files 82 83 +1
Lines 13156 13369 +213
==========================================
+ Hits 7588 7733 +145
- Misses 4991 5037 +46
- Partials 577 599 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
thomas11
force-pushed
the
tkappler/parametrize
branch
from
f2266e3 to
f1feb3d
Compare
thomas11
changed the title
| } | ||
| return ¶meterizeArgs{ | ||
| Module: args[0], | ||
| Version: args[1], // TODO be more accepting of version format and convert |
There was a problem hiding this comment.
The version is definitely worth validating a little more up front to only the formats we accept. Though it will fail later when we try to find the spec, it would probably have a nicer error from here.
There was a problem hiding this comment.
I'm planning to accept a few formats and then convert them to the v20xxxxxx we need for lookups.
| // | ||
| // To separate concerns, the `Parameterization` of the new schema is NOT populated yet, the caller is responsible for | ||
| // doing that. | ||
| func createSchema(p *azureNativeProvider, schema pschema.PackageSpec, targetModule, targetApiVersion string) (*pschema.PackageSpec, *resources.APIMetadata, error) { |
There was a problem hiding this comment.
A general thought here .. we should measure how fast this step is and how much memory the schema consumes in memory. I'd guess it's not too much, but if we wanted to make this a little more efficient, we could just generate the metadata and delay doing the schema until we call GetSchema. Might not be worth the effort though if this is only taking a fraction of a second.
There was a problem hiding this comment.
Good call.
I added runtime.GC() at the top and pprof.Lookup("heap") at the end of Parameterize. From go tool pprof:
(pprof) focus=github.com/pulumi/pulumi-azure-native/v2/provider/pkg/provider.TestParameterizeCreatesSchemaAndMetadata
(pprof) top10
Active filters:
focus=github.com/pulumi/pulumi-azure-native/v2/provider/pkg/provider.TestParamet…
Showing nodes accounting for 160.35MB, 40.29% of 398.03MB total
flat flat% sum% cum cum%
159.20MB 40.00% 40.00% 159.20MB 40.00% os.readFileContents
1.16MB 0.29% 40.29% 1.16MB 0.29% runtime/pprof.StartCPUProfile
0 0% 40.29% 1.16MB 0.29% github.com/pulumi/pulumi-azure-native/v2/provider/pkg/provider.(*azureNativeProvider).Parameterize
0 0% 40.29% 160.35MB 40.29% github.com/pulumi/pulumi-azure-native/v2/provider/pkg/provider.TestParameterizeCreatesSchemaAndMetadata
Looks like Parameterize itself only uses 1.16MB, the rest is reading the full schema in the test. (Please double-check my reasoning.)
For CPU, I used StartCPUProfile/StopCPUProfile. From go tool pprof:
0 0% 28.97% 0.62s 28.97% github.com/pulumi/pulumi-azure-native/v2/provider/pkg/provider.(*azureNativeProvider).Parameterize
Looks harmless to me although I'm not exactly a pprof expert.
There was a problem hiding this comment.
Coolio, yeah that looks fine .. can always dig into optimisations later .. e.g. if we find people wanting to use 20 different parameterizations or something and running out of memory - perhaps we can purge the old schema and metadata.
| v := strings.TrimSpace(version) | ||
| v = strings.TrimLeft(v, "vV") | ||
|
|
||
| isApiVersion, err := regexp.MatchString(`^20\d{2}-[01]\d-[0-3]\d$`, v) |
There was a problem hiding this comment.
Sidethought: this would probably be a useful function to live in the openapi module
There was a problem hiding this comment.
Oh, does this handle all the preview/private-preview variations?
There was a problem hiding this comment.
It does now :)
Would be cool to move this to the openapi package but there are some other concerns there, like handling * wildcards and the empty string meaning the default version. Punting.
thomas11
force-pushed
the
tkappler/parametrize
branch
from
2607917 to
2d29cbb
Compare
|
This PR has been shipped in release v2.89.2. |
6 participants
I picked an approach that's, I hope, pragmatic rather than elegant, in that I assume we can keep the full schema and metadata available for the provider and generate dedicated versioned schemas simply by picking the required parts from the full schema. This avoids having to publish and download or clone data at generation time. A more dynamic approach is equally possible, of course, and could be swapped in without changing the interface.
To understand the PR you should be familiar with these docs.
The input and behavior of Parameterize is the same in the initial "args" call of Parameterize and in the subsequent "serialized parameters" calls: a pair of (Azure module, API version). The existing full schema and metadata are narrowed down to members belonging to the given module at the given version.
Implementation note on
resources.PartialAzureAPIMetadataandresources.APIMetadata: the provider used to store metadata in aPartialAzureAPIMetadata. During Parameterize, we couldn't replace this metadata with the new one which was of typeAzureAPIMetadata. Using the newAPIMetadatawe can accomodate both because it usesMapLikewhich can be a full or a partial map. It turned out that this change also simplified some other code using these types.Related to #2467