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

fix: properly determine list objects in schemagen #3205

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

rquitales
Copy link
Member

@rquitales rquitales commented Sep 14, 2024

CRDs can have types that end in List. These are not actually List resources, so we should not rely on name alone to determine this. Instead, we should follow the rules set out in the conventions guide to determine is a Kind is meant to be a List kind.

Copy link
Contributor

@blampe blampe left a comment

Choose a reason for hiding this comment

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

Deja vu :P

Copy link

Does the PR have any schema changes?

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

Copy link
Contributor

@blampe blampe left a comment

Choose a reason for hiding this comment

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

Is it feasible to test this?

@rquitales
Copy link
Member Author

Is it feasible to test this?

Yes, I'll add to this PR next week!

@rquitales rquitales force-pushed the rquitales/fix-patch-for-list branch from 08cb054 to cc6ef93 Compare September 16, 2024 21:09
@rquitales rquitales added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Sep 16, 2024
@rquitales rquitales force-pushed the rquitales/fix-patch-for-list branch from cc6ef93 to d33c9ff Compare September 16, 2024 21:10
@@ -693,6 +705,14 @@ func createGroups(definitionsJSON map[string]any, allowHyphens bool) []GroupConf

return fmt.Sprintf("%s/%s", gStripped, v)
}

// These resources are hard-coded as lists as they do not adhere to the normal conventions.
Copy link
Member Author

Choose a reason for hiding this comment

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

The list is nested under the resources and groups field instead of items for these two kinds.


-- kinds --
- k8sversion.test.TestResource
- k8sversion.test.TestResourceNotRealList
Copy link
Member Author

Choose a reason for hiding this comment

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

A custom resource that ends in List, but is not a List kind.

@rquitales rquitales requested a review from blampe September 16, 2024 21:13
@rquitales rquitales self-assigned this Sep 16, 2024
Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.

Project coverage is 40.84%. Comparing base (7155e2e) to head (d33c9ff).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
provider/pkg/gen/typegen.go 80.00% 1 Missing and 1 partial ⚠️
provider/pkg/gen/schema.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3205      +/-   ##
==========================================
+ Coverage   38.60%   40.84%   +2.23%     
==========================================
  Files          84       84              
  Lines        9866     9876      +10     
==========================================
+ Hits         3809     4034     +225     
+ Misses       5695     5459     -236     
- Partials      362      383      +21     

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

@rquitales rquitales enabled auto-merge (squash) September 16, 2024 22:54
@rquitales rquitales merged commit 07c064d into master Sep 16, 2024
21 checks passed
@rquitales rquitales deleted the rquitales/fix-patch-for-list branch September 16, 2024 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants