-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Avoid a StackOverflow and properly fail on cyclic references in CRD generation #3652
Conversation
Can one of the admins verify this patch? |
f500a70
to
fdd5c66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good first step but I think that the issue should be addressed at the sundrio
level… What do you think @iocanel ?
crd-generator/api/src/main/java/io/fabric8/crd/generator/AbstractJsonSchema.java
Outdated
Show resolved
Hide resolved
@metacosm I'm not sure that the underlying issue is in |
4586504
to
d35933e
Compare
kubernetes-tests/src/test/java/io/fabric8/crd/generator/zookeeper/v1alpha1/Zookeeper.java
Outdated
Show resolved
Hide resolved
Moving this PR to Draft since I found more edge cases. |
crd-generator/api/src/test/java/io/fabric8/crd/example/nocyclic/NoCyclicStatus.java
Outdated
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed! |
@metacosm this should be ready for a final review now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cc. @manusa @rohanKanojia this should e ready to merge AFAIU. |
From the code, I understand that the fix consists on detecting cyclic references and throwing a human readable exception with the identification of the problematic field/entity in case on is found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx!
Description
This is an initial improvement for #3651 , it mainly adds the failing test and output some human friendly information.
I'm still eager to discuss a proper solution for the underlying issue.
Type of change
test, version modification, documentation, etc.)
Checklist