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

[v3] Detect resource path conflicts and disambiguate #3817

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

thomas11
Copy link
Contributor

@thomas11 thomas11 commented Dec 24, 2024

This PR is based on #3815 and contains its commits.

Resolves #2495
Resolves #2753

Previously, schema and metadata generation of this provider assumed that a type token like azure-native:netapp:Pool uniquely identifies a resource. In the Azure spec, that's not always the case across API versions.

For instance, as shown by #2753, resources such as PrivateEndpointConnection can point to the /servers/ API in one API version but to the newer /flexibleServers/ API, which is a completely different product underneath, in another version.

This PR does two things:

  1. Detect such conflicts. So far, it only prints a warning when it finds one, but when all such conflicts are investigated and fixed it should error.
  2. Disambiguate known instances of such conflicts. In the above example of PrivateEndpointConnection, we rename the resource to SingleServerPrivateEndpointConnection when it points to the /servers/ API.

Any change to the generated schema is gated behind a "version >= 3" check to avoid breaking changes in v2.

Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Copy link

codecov bot commented Dec 24, 2024

Codecov Report

Attention: Patch coverage is 47.78761% with 59 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@8a269f4). Learn more about missing BASE report.

Files with missing lines Patch % Lines
provider/pkg/gen/schema.go 51.92% 45 Missing and 5 partials ⚠️
provider/pkg/versioning/build_schema.go 0.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3817   +/-   ##
=========================================
  Coverage          ?   56.74%           
=========================================
  Files             ?       79           
  Lines             ?    12351           
  Branches          ?        0           
=========================================
  Hits              ?     7009           
  Misses            ?     4814           
  Partials          ?      528           

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

@thomas11 thomas11 force-pushed the tkappler/path-conflicts branch from 317863e to 284e727 Compare December 24, 2024 14:32
@thomas11 thomas11 force-pushed the tkappler/path-conflicts branch from 284e727 to a5fa911 Compare December 31, 2024 07:20
@thomas11 thomas11 requested review from danielrbradley, a team and EronWright December 31, 2024 07:23
@thomas11 thomas11 force-pushed the tkappler/path-conflicts branch from a5fa911 to ea458e3 Compare January 2, 2025 19:07
@thomas11 thomas11 force-pushed the tkappler/path-conflicts branch from ea458e3 to d3af70c Compare January 6, 2025 08:55
canonPath := paths.NormalizePath(resource.Path)

typeName := resource.typeName
if g.majorVersion > 3 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

>= !!

@danielrbradley
Copy link
Member

danielrbradley commented Jan 8, 2025

Summary from conversation: add typed strings for ModuleName, ResourceName and ResourcePath; and make the report a map[ModuleName]map[ResourceName]map[ResourcePath]any.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants