Refactor deck to use /prowjobs.js #7301

kpatticha · 2018-03-15T18:23:27Z

The PR is mainly refactoring of script.js in order to use /prowjobs.js .

The only addition is the calculation of the job's duration which it is not included in the new response.

k8s-ci-robot · 2018-03-15T18:23:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: katerinapat
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: cjwagner

Assign the PR to them by writing /assign @cjwagner in a comment when ready.

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:

prow/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

stevekuznetsov · 2018-03-15T18:46:20Z

/ok-to-test

stevekuznetsov · 2018-03-15T18:46:51Z

/cc @BenTheElder @qhuynh96

stevekuznetsov · 2018-03-15T18:48:43Z

prow/cmd/deck/static/script.js

+        completionMoment = new moment(completionTime),
+        d = moment.duration(completionMoment.diff(startMoment));
+
+    return d.hours() + "h" + d.minutes() + "m" + d.seconds() + "s";


d.format() ?

Duration object seems not to have format().

Yeah, see moment/moment#1048.

This is wrong for jobs that take too long, and it will look weird to have a bunch of 0h jobs.

var out = d.seconds() + 's'; if (d.asHours() >= 1) out = Math.floor(d.asHours()) + 'h' + d.minutes() + 'm' + out; else if (d.minutes() >= 1) out = d.minutes() + 'm' + out; return out;

stevekuznetsov · 2018-03-15T18:49:15Z

prow/cmd/deck/static/script.js

 function createTimeCell(id, time) {
-    var momentTime = moment.unix(time);


Not immediately obvious why this was wrong?

stevekuznetsov · 2018-03-15T18:49:51Z

prow/cmd/deck/static/script.js

+        if (!repo || repo === build.spec.refs.repo) {
+            opts.jobs[build.spec.job] = true;
+            if (build.spec.type === "presubmit") {
+                opts.authors[build.spec.refs.pulls[0]['author']] = true;


Do we want to not squash the list of pulls for batches?

is there any case that build.spec.refs.pulls can be null or undefined?

Yes, in periodic and postsubmit jobs there will be no pulls

qhuynh96 · 2018-03-16T17:11:48Z

prow/cmd/deck/static/script.js

+        if (!repo || repo === build.spec.refs.repo) {
+            opts.jobs[build.spec.job] = true;
+            if (build.spec.type === "presubmit") {
+                opts.authors[build.spec.refs.pulls[0]['author']] = true;


is there any case that build.spec.refs.pulls can be null or undefined?

qhuynh96 · 2018-03-16T17:22:13Z

prow/cmd/deck/static/script.js

+            build.spec.refs = prop;
+        } 
+
+        if (!build.spec.refs.hasOwnProperty("pulls")) {


is there any case that build.spec.refs inherit pulls? if not we can change the condition to !build.spec.refs.pulls

qhuynh96 · 2018-03-16T17:45:54Z

prow/cmd/deck/static/script.js

+            ]
+        }
+
+        if ( Object.keys(build.spec.refs).length == 0){


this should be explicitly periodic type job as we expect postsubmit presubmit and batch to have refs; otherwise, revision cells will be filled with broken links.

If postsubmit, presubmit or batch does not have refs, should we consider to drop it?

I am inclined to say just drop them but if we don't do that today I don't consider it important fixing here.

0xmichalis · 2018-03-23T01:40:42Z

Looks OK overall to me. There have been some concerns raised by @rmmh and @BenTheElder in the past wrt the size of prowjobs.js. Maybe we can pass a url parameter that would drop pod specs from the response?

0xmichalis · 2018-03-23T02:24:20Z

Looks OK overall to me. There have been some concerns raised by @rmmh and @BenTheElder in the past wrt the size of prowjobs.js. Maybe we can pass a url parameter that would drop pod specs from the response?

Opened #7386

rmmh · 2018-03-23T21:55:52Z

prow/cmd/deck/static/script.js

-                opts.authors[build.author] = true;
-                opts.pulls[build.number] = true;
-                opts.states[build.state] = true;
+    for (var i = 0; i < allBuilds.items.length; i++) {


Since we're using ES6 now: for (const build of allBuilds.items) {

rmmh · 2018-03-23T21:56:27Z

prow/cmd/deck/static/script.js

-                opts.pulls[build.number] = true;
-                opts.states[build.state] = true;
+    for (var i = 0; i < allBuilds.items.length; i++) {
+        var build = allBuilds.items[i];


const spec = build.spec;, to make the rest shorter?

rmmh · 2018-03-23T22:02:25Z

prow/cmd/deck/static/script.js

+            opts.jobs[build.spec.job] = true;
+            if (build.spec.type === "presubmit") {
+                opts.authors[build.spec.refs.pulls[0]['author']] = true;
+                opts.pulls[build.spec.refs.pulls[0]['number']] = true;


pulls[0].number

rmmh · 2018-03-23T22:05:37Z

prow/cmd/deck/static/script.js

@@ -353,7 +353,11 @@ function equalSelected(sel, t) {
 }

 function groupKey(build) {
-    return build.repo + " " + build.number + " " + build.refs;
+    return build.spec.refs.org + "/" +


This won't do the right grouping for batch jobs. Try this:

const refs = build.spec.refs; const pulls = refs.pulls.map(r => r.number + ':' + r.sha).join(','); return `${refs.repo} ${refs.base_ref}:${refs.base_sha},${pulls}`;

rmmh · 2018-03-23T22:20:50Z

prow/cmd/deck/static/script.js

+        completionMoment = new moment(completionTime),
+        d = moment.duration(completionMoment.diff(startMoment));
+
+    return d.hours() + "h" + d.minutes() + "m" + d.seconds() + "s";


Yeah, see moment/moment#1048.

This is wrong for jobs that take too long, and it will look weird to have a bunch of 0h jobs.

var out = d.seconds() + 's'; if (d.asHours() >= 1) out = Math.floor(d.asHours()) + 'h' + d.minutes() + 'm' + out; else if (d.minutes() >= 1) out = d.minutes() + 'm' + out; return out;

rmmh · 2018-03-23T22:22:27Z

prow/cmd/deck/static/script.js

+        var build = allBuilds.items[i];
+
+        // assign default values 
+        var prop = { 


I would skip this, and instead change the one case that uses build.spec.refs to skip if build.spec.refs is empty.

rmmh · 2018-03-23T22:25:27Z

prow/cmd/deck/static/script.js

@@ -26,16 +26,16 @@ function optionsForRepo(repo) {
        states: {},
    };


Why not do an initialization pass to set build.author and build.number, and avoid the construct of build.spec.refs.pulls[0] elsewhere?

This should still be done-- getting the hnumber out should be simpler.

0xmichalis · 2018-04-16T10:48:18Z

@katerinapat is this ready for another round of reviews?

…use-prowjob

stevekuznetsov · 2018-04-16T16:01:51Z

When we get closer to merge, please rebase your branch on top of master instead of merging.

kpatticha · 2018-04-16T17:43:28Z

@Kargakis. I pushed 2 more commits. Added the param omit=pod_spec on the request of prowjobs.js and I dropped the default values. Instead I check the cases that the values could not be set.

I'll refactor the duration so it won't display all these 0h.

Although in this PR I didn't include any other refactoring or changing to ES6. Main goal is to refactor the script to support the new response without brake anything. I'll be more happy to refactor the whole spaghetti code in a future PR

fejta · 2018-06-08T00:59:34Z

/assign @Kargakis @rmmh
/lifecycle stale

Still working on this one?

fejta · 2018-06-18T23:56:13Z

Please address comments and reopen!
/close

kpatticha requested review from cjwagner and stevekuznetsov as code owners March 15, 2018 18:23

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 15, 2018

k8s-ci-robot requested review from BenTheElder and qhuynh96 March 15, 2018 18:46

stevekuznetsov reviewed Mar 15, 2018

View reviewed changes

qhuynh96 reviewed Mar 16, 2018

View reviewed changes

rmmh suggested changes Mar 23, 2018

View reviewed changes

0xmichalis mentioned this pull request Mar 23, 2018

Use ProwJobs in deck #5216

Closed

5 tasks

Initial refactoring to start using /prowjob.js in deck

100e818

kpatticha force-pushed the refactor-deck-to-use-prowjob branch from d4d8937 to 5e014da Compare April 15, 2018 11:24

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2018

Katerina Patticha added 2 commits April 15, 2018 13:36

Fix start time display

9a6b7a9

Add duration of the job

9f52ad9

kpatticha force-pushed the refactor-deck-to-use-prowjob branch from 5e014da to 9f52ad9 Compare April 15, 2018 11:37

Merge remote-tracking branch 'upstream/master' into refactor-deck-to-…

01250df

…use-prowjob

Katerina Patticha added 2 commits April 16, 2018 19:19

Request prowjobs.js data without pod specs

343ebb2

Drop default values and fix the undefined errors

2127bd0

k8s-ci-robot assigned 0xmichalis Jun 8, 2018

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 8, 2018

k8s-ci-robot assigned rmmh Jun 8, 2018

k8s-ci-robot closed this Jun 18, 2018

clarketm mentioned this pull request Aug 29, 2019

Refactor deck to use prowjob #14126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor deck to use /prowjobs.js #7301

Refactor deck to use /prowjobs.js #7301

kpatticha commented Mar 15, 2018 •

edited by 0xmichalis

k8s-ci-robot commented Mar 15, 2018

stevekuznetsov commented Mar 15, 2018

stevekuznetsov commented Mar 15, 2018

stevekuznetsov Mar 15, 2018

qhuynh96 Mar 16, 2018

rmmh Mar 23, 2018

stevekuznetsov Mar 15, 2018

stevekuznetsov Mar 15, 2018

qhuynh96 Mar 16, 2018

stevekuznetsov Mar 16, 2018

qhuynh96 Mar 16, 2018

qhuynh96 Mar 16, 2018

qhuynh96 Mar 16, 2018

0xmichalis Mar 23, 2018

0xmichalis commented Mar 23, 2018

0xmichalis commented Mar 23, 2018

rmmh Mar 23, 2018

rmmh Mar 23, 2018

rmmh Mar 23, 2018

rmmh Mar 23, 2018

rmmh Mar 23, 2018

rmmh Mar 23, 2018

rmmh Mar 23, 2018

rmmh Apr 16, 2018

0xmichalis commented Apr 16, 2018

stevekuznetsov commented Apr 16, 2018

kpatticha commented Apr 16, 2018

fejta commented Jun 8, 2018

fejta commented Jun 18, 2018

		function createTimeCell(id, time) {
		var momentTime = moment.unix(time);

		@@ -26,16 +26,16 @@ function optionsForRepo(repo) {
		states: {},
		};

Refactor deck to use /prowjobs.js #7301

Refactor deck to use /prowjobs.js #7301

Conversation

kpatticha commented Mar 15, 2018 • edited by 0xmichalis

k8s-ci-robot commented Mar 15, 2018

stevekuznetsov commented Mar 15, 2018

stevekuznetsov commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Mar 23, 2018

0xmichalis commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Apr 16, 2018

stevekuznetsov commented Apr 16, 2018

kpatticha commented Apr 16, 2018

fejta commented Jun 8, 2018

fejta commented Jun 18, 2018

kpatticha commented Mar 15, 2018 •

edited by 0xmichalis