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
Bug 1830841: updates api-endpoint to fetch event sources and update default sources #5270
Bug 1830841: updates api-endpoint to fetch event sources and update default sources #5270
Conversation
@invincibleJai: This pull request references Bugzilla bug 1830841, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
frontend/packages/knative-plugin/src/utils/fetch-dynamic-eventsources-utils.ts
Outdated
Show resolved
Hide resolved
frontend/packages/knative-plugin/src/utils/fetch-dynamic-eventsources-utils.ts
Outdated
Show resolved
Hide resolved
frontend/packages/knative-plugin/src/utils/fetch-dynamic-eventsources-utils.ts
Outdated
Show resolved
Hide resolved
bbd38a4
to
1b56df7
Compare
New API is working fine for both consoledeveloper(basic user) and kubeadmin when logged in using the cluster URL and on the localhost, API is throwing |
@sahil143 This is an expected behaviour. In off-cluster mode, |
const modelData = version | ||
? { | ||
apiGroup: group, | ||
apiVersion: version, | ||
kind, | ||
plural, | ||
id: singular, | ||
label: singular, | ||
labelPlural: plural, | ||
abbr: kindToAbbr(kind), | ||
namespaced: true, | ||
crd: true, | ||
color: knativeEventingColor.value, | ||
} | ||
: {}; | ||
return [...accumulator, modelData]; |
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.
Its better to just push instead of spreading the array in these scenarios. Its much faster that spreading and does not create a whole new array again every time.
const modelData = version | |
? { | |
apiGroup: group, | |
apiVersion: version, | |
kind, | |
plural, | |
id: singular, | |
label: singular, | |
labelPlural: plural, | |
abbr: kindToAbbr(kind), | |
namespaced: true, | |
crd: true, | |
color: knativeEventingColor.value, | |
} | |
: {}; | |
return [...accumulator, modelData]; | |
if (version) { | |
accumulator.push({ | |
apiGroup: group, | |
apiVersion: version, | |
kind, | |
plural, | |
id: singular, | |
label: singular, | |
labelPlural: plural, | |
abbr: kindToAbbr(kind), | |
namespaced: true, | |
crd: true, | |
color: knativeEventingColor.value, | |
}); | |
} | |
return accumulator; |
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.
The older approach also has a problem where you would add empty objects to the accumulator when there is no version available. So your final array would look some thing like [model1, model2, {}, {}]
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.
I agree with Rohit. Sorry, I missed this in my review.
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.
@rohitkrai03 @sahil143 updated PTAL
1b56df7
to
0ed9818
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.
/lgtm
verified changes locally
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: invincibleJai, sahil143 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
@invincibleJai: All pull requests linked via external trackers have merged: openshift/console#5270. Bugzilla bug 1830841 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@invincibleJai: Bugzilla bug 1830841 is in an unrecognized state (MODIFIED) and will not be moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Fixes:
https://issues.redhat.com/browse/ODC-3683
https://issues.redhat.com/browse/ODC-3775
Analysis / Root cause:
Currently fetching dynamic sources with making direct call to CRD with version
v1beta
based on label selector ducktypeSolution Description:
Update ApiEnd point to fetch CRDs for event sources (api using CRD version "v1") i.e
api/console/knative-event-sources
and return Default Sources in orderScreen shots / Gifs for design review:
Browser conformance: