Skip to content

Bug 1620368 - Remove incorrect fields from cron job page#446

Merged
spadgett merged 1 commit intoopenshift:masterfrom
spadgett:cron-job-details
Aug 23, 2018
Merged

Bug 1620368 - Remove incorrect fields from cron job page#446
spadgett merged 1 commit intoopenshift:masterfrom
spadgett:cron-job-details

Conversation

@spadgett
Copy link
Member

@spadgett spadgett commented Aug 23, 2018

Job status will not be set in the cron job resource. You have to look at
an individual job to see status. Remove these fields from the cron job page.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1620368

example details okd 2018-08-23 08-45-03

/assign @jhadvig

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 23, 2018
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 _.get calls are unnecessary, and this would already fail with a runtime error if spec was undefined at line 46.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that you can specify cronjob without spec field

@spadgett
Copy link
Member Author

/hold

I realize we're also showing name and namespace twice

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2018
Job status will not be set in the cron job resource. You have to look at
an individual job to see status. Remove these fields from the cron job page.
@spadgett
Copy link
Member Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2018
@spadgett
Copy link
Member Author

Opened https://jira.coreos.com/browse/CONSOLE-736 to add some detail about the container being run.

<dd>{job.spec.parallelism || '-'}</dd>
<dt>Deadline</dt>
<dd>{job.spec.activeDeadlineSeconds ? `${job.spec.activeDeadlineSeconds} seconds` : '-'}</dd>
<dt>Status</dt>
Copy link
Member

Choose a reason for hiding this comment

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

I think that it also would make sense to show restartPolicy, mainly cause this field is always present...

Copy link
Member Author

Choose a reason for hiding this comment

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

That is part of the pod template, and we're not showing any info about the containers currently.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened CONSOLE-736 to add container info

Copy link
Member

Choose a reason for hiding this comment

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

makes sense, just wasnt sure if we want to address this in this PR or in a separate one, but since there is a story for that it does makes sense to add it as part of it 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it's a great point, and I think generally the workloads pages need more detail. We should be careful how we do it though because pod templates have a lot of fields that can clutter the UI, so I didn't want to do too much in this PR.

Thanks @jhadvig

Copy link
Member

@jhadvig jhadvig left a comment

Choose a reason for hiding this comment

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

\lgtm

@spadgett spadgett merged commit 52d4258 into openshift:master Aug 23, 2018
@spadgett spadgett deleted the cron-job-details branch August 23, 2018 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants