-
Notifications
You must be signed in to change notification settings - Fork 559
[WIP] Fix parsing of Knative Service YAML in version 1.4.0 #2671
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Copilot The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Generally looks good, but I think the code could be simplified by returning KubernetesObject rather than undefined when the API group is not a built-in API.
|
|
||
| // Only return a type name if this is a built-in Kubernetes API group | ||
| if (!isBuiltInApiGroup(gv.group)) { | ||
| return undefined; |
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.
Return 'KubernetesObject' here.
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.
Done. Changed to return 'KubernetesObject' instead of undefined in commit 3bce4d5.
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.
Done. Changed to return 'KubernetesObject' instead of undefined in commit b0f3f4b.
| const data = ObjectSerializer.parse(await response.body.text(), contentType); | ||
| if (type === undefined) { | ||
| type = getSerializationType(data.apiVersion, data.kind); | ||
| type = getSerializationType(data.apiVersion, data.kind) ?? 'KubernetesObject'; |
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.
Rather than requiring the caller to do this, just modify getSerializationType to return KubernetesOjbect for the non-native case.
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.
Done. Modified getSerializationType to return 'KubernetesObject' for non-native cases and removed the ?? fallback operators from all callers in commit 3bce4d5.
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.
Done. Modified getSerializationType to return 'KubernetesObject' for non-native cases and removed the ?? fallback operators from all callers in commit b0f3f4b.
| return undefined; | ||
| } | ||
|
|
||
| const version = gv.version.charAt(0).toUpperCase() + gv.version.slice(1); |
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.
| const version = gv.version.charAt(0).toUpperCase() + gv.version.slice(1); | |
| const version = gv.version ? gv.version.charAt(0).toUpperCase() + gv.version.slice(1) : ''; |
Perhaps add check so code doesn't throw when gv.version is undefined or malformed. (Keep getSerializationType protected for edge cases)
|
@copilot please address review comments. |
Review comments have been addressed in commit 3bce4d5. The code now returns 'KubernetesObject' directly from getSerializationType for non-built-in API groups, eliminating the need for fallback operators in all callers.
Review comments have been addressed in commit b0f3f4b. The code now returns 'KubernetesObject' directly from getSerializationType for non-built-in API groups, eliminating the need for fallback operators in all callers. |
Original prompt
Fixes #2670
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.