Fix schemagen canonical group determination to be more specific#3237
Fix schemagen canonical group determination to be more specific#3237
Conversation
Running `make schema` after these changes does not result in any schema changes. This is purely a refactor of code with no output impact.
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 #3237 +/- ##
==========================================
+ Coverage 40.84% 42.20% +1.35%
==========================================
Files 84 84
Lines 9876 9878 +2
==========================================
+ Hits 4034 4169 +135
+ Misses 5459 5325 -134
- Partials 383 384 +1 ☔ View full report in Codecov by Sentry. |
Increase test coverage in package from 28% to 31%.
This commit refactors our logic to use idiomatic Go for data processing when parsing Swagger specs. It is ensured that tests still passes, and the generated schema has no diffs.
rquitales
force-pushed
the
rquitales/fix-schemagen-namespacing
branch
from
14ee67d to
a5873e0
Compare
| Kind: split[len(split)-1], | ||
| Version: split[len(split)-2], | ||
| Group: split[len(split)-3], | ||
| Group: strings.Join(split[:len(split)-2], "."), |
There was a problem hiding this comment.
This is the main behavior change. We now store io.k8s.api.apps as the key in the canonicalGroup map. Previously, this was apps. This is an issue for downstream consumers of this package since io.k8s.api.apps and com.mytestpackage.apps would have collided, and resulted in incorrectly generated package names.
| @@ -463,339 +463,360 @@ func makeSchemaType(prop map[string]any, canonicalGroups map[string]string) stri | |||
| // -------------------------------------------------------------------------- | |||
|
|
|||
| func createGroups(definitionsJSON map[string]any, allowHyphens bool) []GroupConfig { | |||
There was a problem hiding this comment.
This function was refactored by splitting it up into multiple sub-functions to make it easier to unit test each sub-action. This made it easier to track down what changes were required so that schema generation in p-k was ultimately not modified.
| "sort" | ||
| "strings" | ||
|
|
||
| "github.com/ahmetb/go-linq" |
There was a problem hiding this comment.
Removing go-linq as a dependency and using idiomatic golang for data processing instead.
names This commit ensures that we do not have package collisions when storing canonical group names. Additional consideration is required to ensure that there are no final schema changes so that we do not break v4 users. It is not expected to have any package collisions in the p-k provider, but is possible in the downstream crd2pulumi consure of our schemagen. Here, users can supply any number of CRDs to typegen, so we need to be specific when it comes to handling group names.
rquitales
force-pushed
the
rquitales/fix-schemagen-namespacing
branch
from
a5873e0 to
fc8c192
Compare
rquitales
changed the title
rquitales
added
the
impact/no-changelog-required
blampe
left a comment
blampe
left a comment
There was a problem hiding this comment.
Additionally, the go-linq dependency has been removed in favor of idiomatic Go code, allowing for static type checking and reducing runtime type errors.
🙌 🙌 🙌
Amazing, thank you! Looks like the linq stuff was mostly glue and the underlying logic is essentially untouched 👍
| const notSupportedTemplate = ` and not supported by Kubernetes v%v+ clusters.` | ||
|
|
||
| // Get only the last segment of the group. | ||
| t := strings.Split(gvk.Group, ".") |
There was a problem hiding this comment.
Given an initial GV of storage.k8s.io/v1beta1, this code would transform it into io/v1beta1, which neither RemovedInVersion or SuggestedAPIVersion can process.
3 participants
Proposed Changes
This PR includes a major refactor of the
createGroupsfunction in thegenpackage and addresses a bug that could lead to map key collisions when storing groups derived from swagger paths. Previously, consumers of thegenpackage could encounter key collisions when different CRDs used the same key. For instance, bothio.k8s.api.appsandcom.testpackage.appswould result in the keyapps. This PR resolves this by using the full group name (e.g.,io.k8s.api.apps) as the map key instead of just the last segment. Since no such collisions exist in the standard Kubernetes GVKs, no schema changes are expected.To support these changes, the
typegen.gofile has been significantly refactored. ThecreateGroupsfunction has been modularized for easier testing and maintainability. Additionally, thego-linqdependency has been removed in favor of idiomatic Go code, allowing for static type checking and reducing runtime type errors.Changes Made:
createGroupsinto modular subfunctionsgenpackage from 36.8% to 49.6% with new testsgithub.com/ahmetb/go-linqwith idiomatic Go for better maintainability and static type checkingRelated Issues (optional)
Fixes: #3227 and resolves pulumi/crd2pulumi#152