V3 alias refactoring #3872
V3 alias refactoring #3872
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3872 +/- ##
==========================================
+ Coverage 57.12% 57.26% +0.13%
==========================================
Files 79 79
Lines 12423 12469 +46
==========================================
+ Hits 7097 7140 +43
Misses 4799 4799
- Partials 527 530 +3 ☔ View full report in Codecov by Sentry. |
danielrbradley
force-pushed
the
v3-alias-preparations
branch
from
a813e27 to
dd31aac
Compare
Use the full word for clarity (and make my spell checker happy)!
Include spec file path and version information so it's available for context when manipulating the specification into the schema.
- Use a proper struct to represent all the information we parse to decide the name. - Fix matching on mixed-case folder names. - Add test case to cover mismatched namespace from file path vs spec path.
Separate alias logic from main genResources method.
This is the name we use for organisation into modules, not how Azure organises it. These modules are only sometimes, but not always based on the Azure "Resource Provider" name. - Change type aliases to type definitions. - Rename associated variables. - Switch from codegen.SortedKeys to util.SortedKeys which is generic.
API version is convertable to SDK version easier than the other way round so keep it as API version as much as possible. - Add helper functions to ApiVersion type.
danielrbradley
force-pushed
the
v3-alias-preparations
branch
from
dd31aac to
e07184f
Compare
We want the whole namespace - with the prefix. Therefore we need to copy the original findNamespaceWithoutPrefixFromPath method into a method that doesn't remove the prefix. - Validate the newly extracted namespace against the old method of extraction. - Removing the old method of extraction is very tricky as it's not clear what's bugs or desired behaviour when skipping resources. - Add test coverage for "well known" namespaces
danielrbradley
force-pushed
the
v3-alias-preparations
branch
from
e07184f to
c6742ee
Compare
- "Lock" wasn't an accurate description as it's always regenerated from the spec. - Rename "DefaultConfigToDefaultVersionLock" to "DefaultVersionsFromConfig"
- Name as the major version number as the whole folder is the "default versions" files and this is the main result. - Use yaml over json for less visual noise and so we can include a warning about being generated. - Manually converted v1 & verified that the data in v2 is identical.
| } | ||
| } | ||
| return "", nil, false | ||
| } | ||
|
|
||
| // Identify the first segment of the path that contains a namespace. |
There was a problem hiding this comment.
Isn't taking the first namespace a change in behavior? Previously, the code used findNamespaceWithoutPrefixFromPath which takes the last.
There was a problem hiding this comment.
So, I think the old logic is wrong (working right to left) and only works because we're only considering the explicit prefixes "Microsoft." and "PaloAltoNetworks.". For resource URIs, there's other segments with dots (in Microsoft.EventGrid has segments like .../{a}.{b}/...).
I left the old method in with an inline comparison to prove that these approaches are equivelent for the time being:
// Sanity check the new and old methods return consistent results for now.
I don't like having two methods doing nearly the same thing. If we're confident the new approach is solid, then we can take out the old approach and leave the "less hard-coded" new approach in.
The other half of the broken logic I started addressing then reverted out was the "resolved module name mismatch" errors which seems overly aggressive and is excluding real resources, but I think that needs to be a separate PR as it changes the computed resources.
- Rename versionMap - Document TokenModule - Update docs on version files.
|
This PR has been shipped in release v2.84.0. |
Due to low confidence on the migration path in v2, we added all resource being used in the default version as explicit versions too. With the improved aliasing approach from #3872, this is not required any more. See the preview generation against the v3 branch: v3...dont-duplicate-default-as-explicit-v3-preview This has a net reduction of 1,118,866 lines of SDK code and an uncompressed size reduction of just under 7% to deliver a 48% overall reduction for v3. | SDK | v2 size | v3 size | new v3 size | |-----|-------|------|---------| | dotnet | 653M | 363M | 340M | | nodejs | 300M | 166M | 156M | | python | 717M | 401M | 376M |
Try to get the right information into the right places in order to implement robust auto-aliasing to address #3848