Skip to content
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

Support for viewing completed/inactive CronJob jobs #3103

Merged
merged 5 commits into from
Jun 25, 2018

Conversation

jeefy
Copy link
Member

@jeefy jeefy commented Jun 20, 2018

Hi all! This directly addresses #2786

I created an additional function that pulls only completed Jobs for a given CronJob.
Next I shimmed in "Inactive Jobs" in the CronJobDetail Type as well as in the Angular page under the "Active Jobs" card.

Tested locally via minikube (v1.10, v1.9, v1.8)

Let me know if there's anything I need to change or fix!

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 20, 2018
@jeefy
Copy link
Member Author

jeefy commented Jun 20, 2018

/assign @danielromlein

@codecov
Copy link

codecov bot commented Jun 20, 2018

Codecov Report

Merging #3103 into master will increase coverage by 0.02%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3103      +/-   ##
==========================================
+ Coverage   54.23%   54.25%   +0.02%     
==========================================
  Files         564      564              
  Lines       12209    12254      +45     
==========================================
+ Hits         6621     6648      +27     
- Misses       5332     5346      +14     
- Partials      256      260       +4
Impacted Files Coverage Δ
src/app/backend/resource/cronjob/detail.go 70% <100%> (+3.33%) ⬆️
src/app/backend/resource/cronjob/jobs.go 56.97% <53.84%> (+2.43%) ⬆️
...p/backend/integration/metric/common/aggregation.go 87.69% <0%> (-1.54%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2dabdc...3552218. Read the comment docs.

@danielromlein danielromlein assigned bryk and unassigned danielromlein Jun 21, 2018
Copy link
Contributor

@bryk bryk left a comment

Choose a reason for hiding this comment

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

Great thanks for the contribution!

@bryk
Copy link
Contributor

bryk commented Jun 22, 2018

@pengx17 Can you also review this and LGTM/comment?

@pengx17
Copy link
Contributor

pengx17 commented Jun 25, 2018

/LGTM, btw @jeefy I do not find changes related to regular Jobs. Do you think it is better to include these to Job detail pages as well?

@bryk bryk added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 25, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: jeefy

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 228161b into kubernetes:master Jun 25, 2018
@jeefy
Copy link
Member Author

jeefy commented Jun 25, 2018

Thanks for the feedback/merge!

@pengx17 This is to display jobs owned by a CronJob specifically. It wouldn't apply to a Job itself. If there are more use cases of jobs spawning other jobs (a la HPC workflow) it might make sense.

It might be a good idea to eventually have the ability to display objects that have an OwnerRef of the currently-viewed object. That would probably solve this across the board. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants