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
Topology, fetch jobs for cronjobs when necessary #7164
Topology, fetch jobs for cronjobs when necessary #7164
Conversation
): { loaded: boolean; loadError: string; jobs: JobKind[] } => { | ||
const { namespace, uid } = cronJob.metadata; | ||
const [loaded, setLoaded] = React.useState<boolean>(false); | ||
const [loadError, setLoadError] = React.useState<string>(''); |
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 [loadError, setLoadError] = React.useState<string>(''); | |
const [loadError, setLoadError] = React.useState<string>(null); |
@jeff-phillips-18 Tested this with the CronJob example below and got an error in pods-overview.tsx#L254-L257 because apiVersion: batch/v1beta1
kind: CronJob
metadata:
name: example
namespace: christoph-test
spec:
schedule: '@daily'
jobTemplate:
spec:
template:
spec:
containers:
- name: hello
image: busybox
args:
- /bin/sh
- '-c'
- date; echo Hello from the Kubernetes cluster
restartPolicy: OnFailure So you need to return jobs state in edit: I see the benefit of using a default as well as an check in PodsOverviewMultiple. If you prefer the default we can use also the benefit of the "" default for loadError, in that case let us use setLoadError("") if there is no error. |
84087a2
to
a8549b5
Compare
Thanks @jerolimov, updated. |
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
let updatedPods = podResources?.length | ||
? podResources.reduce((acc, resource) => { | ||
acc.push(...getPodsForResource(resource, resources)); | ||
return acc; | ||
}, []) | ||
: []; |
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 personally would go with this, but its okay 😏
let updatedPods = podResources?.length | |
? podResources.reduce((acc, resource) => { | |
acc.push(...getPodsForResource(resource, resources)); | |
return acc; | |
}, []) | |
: []; | |
let updatedPods = podResources?.reduce((acc, resource) => { | |
acc.push(...getPodsForResource(resource, resources)); | |
return acc; | |
}, []) ?? []; |
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.
Verified locally that it did not crash anymore, changed to setLoadError(""), has already lgtm, and I would give it as well 👍
/lgtm
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.
/approve
a8549b5
to
fd37cf6
Compare
Rebased |
fd37cf6
to
f189dd6
Compare
rebased again |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, dtaylor113, jeff-phillips-18, jerolimov, rohitkrai03 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 |
Fixes:
https://issues.redhat.com/browse/ODC-5115
Solution Description:
Remove jobs from OverviewItem type. Add hook to retrieve jobs for a given cronjob resource. Update instances to use hooks rather than fields in the OverviewItem type.
Non-Functional Change
Browser conformance:
/kind cleanup