Fix: add stale build label if it is removed before updating branch #235

nlok5923 · 2021-03-28T12:18:27Z

Fixes: #225

Explanation

Added script to add stale build label if it was removed from PR before updating the pr.

Checklist

I have successfully deployed my own instance of Oppiabot.
- You can find instructions for doing this here.
I have manually tested all the changes made in this PR following the manual tests matrix.

nlok5923 · 2021-03-28T12:22:09Z

@vojtechjelinek @jameesjohn sir please review this pr.

nlok5923 · 2021-03-28T12:31:36Z

manual testing video:

bot.mp4

vojtechjelinek

Thanks! Left a few comments.

vojtechjelinek · 2021-03-28T16:45:29Z

lib/checkPullRequestLabels.js

+      const commentParams = context.issue({
+        body:
+        'Hi @' + user.login + ', the build of this pr is ' +
+        ' stale please do not remove ' + [OLD_BUILD_LABEL] + 


Suggested change

' stale please do not remove ' + [OLD_BUILD_LABEL] +

' stale please do not remove \'' + OLD_BUILD_LABEL + '\''

addressed in latest commits

vojtechjelinek · 2021-03-28T16:46:03Z

lib/checkPullRequestLabels.js

+      const commentParams = context.issue({
+        body:
+        'Hi @' + user.login + ', the build of this pr is ' +
+        ' stale please do not remove ' + [OLD_BUILD_LABEL] + 
+        ' label. ',
+      });
+      await context.github.issues.createComment(commentParams);


Suggested change

const commentParams = context.issue({

body:

'Hi @' + user.login + ', the build of this pr is ' +

' stale please do not remove ' + [OLD_BUILD_LABEL] +

' label. ',

});

await context.github.issues.createComment(commentParams);

const commentParams = context.issue({

body:

'Hi @' + user.login + ', the build of this pr is ' +

' stale please do not remove ' + [OLD_BUILD_LABEL] +

' label. ',

});

await context.github.issues.createComment(commentParams);

addressed in latest commits

lib/checkPullRequestLabels.js

vojtechjelinek · 2021-03-28T16:49:04Z

lib/checkPullRequestLabels.js

+ * This function checks and prevent the removal of critical label
+ * by unauthorised users.


Suggested change

* This function checks and prevent the removal of critical label

* by unauthorised users.

* This function checks and prevent the removal of stale build label

* by unauthorised users.

addressed in latest commits

jameesjohn

Thanks, @nlok5923. I left some comments.

jameesjohn · 2021-03-28T21:56:49Z

lib/checkPullRequestLabels.js

+    const commentParams = context.issue({
+      body:
+        'Hi @' + user.login + ', the build of this pr is ' +
+        ' stale please do not remove \'' + OLD_BUILD_LABEL + '\'' +
+        ' label. ',
+    });
+    await context.github.issues.createComment(commentParams);


Before commenting on this PR, we need to actually check if the build of the PR is stale.

See

oppiabot/lib/staleBuildChecks.js

Lines 22 to 30 in 959b59c

const {data: lastCommit} = await context.github.repos.getCommit(

context.repo({

ref: pullRequest.head.sha

})

);

const lastCommitDate = new Date(lastCommit.commit.author.date);

// Commit is older than 2 days

if (utils.MIN_BUILD_DATE > lastCommitDate) {

yes sir i agree with you i had added the check in the latest commit.

jameesjohn · 2021-03-28T21:57:35Z

lib/checkPullRequestLabels.js

+ *
+ * @param {import('probot').Context} context
+ */
+module.exports.checkBuildLabelRemoval = async function (context) {


Suggested change

module.exports.checkBuildLabelRemoval = async function (context) {

module.exports.checkStaleBuildLabelRemoval = async function (context) {

changes addressed in latest commit.

jameesjohn · 2021-03-28T21:58:01Z

index.js

@@ -115,6 +115,11 @@ const runChecks = async (context, checkEvent) => {
              checkPullRequestLabelsModule.checkCriticalLabel(context)
            );
            break;
+          case constants.staleBuildLabelCheck:
+            callable.push(
+              checkPullRequestLabelsModule.checkBuildLabelRemoval(context)


Suggested change

checkPullRequestLabelsModule.checkBuildLabelRemoval(context)

checkPullRequestLabelsModule.checkStaleBuildLabelRemoval(context)

changes addressed in latest commit.

nlok5923 · 2021-03-30T17:08:30Z

@vojtechjelinek @jameesjohn sir please review this pull request.

jameesjohn

Thanks, two extra comments.

jameesjohn · 2021-03-30T23:40:27Z

lib/checkPullRequestLabels.js

+ *
+ * @param {import('probot').Context} context
+ */
+module.exports.checkStaleBuildLabelRemoval = async function (context) {


No tests are written for this function.

added test in latest commit

jameesjohn · 2021-03-30T23:42:11Z

lib/checkPullRequestLabels.js

+    const {data: lastCommit} = await context.github.repos.getCommit(
+      context.repo({
+        ref: pullRequest.head.sha
+      })
+    );
+
+    const lastCommitDate = new Date(lastCommit.commit.author.date);
+    // Commit is older than 2 days
+    if (MIN_BUILD_DATE > lastCommitDate) {


Since we are making this check at multiple locations, this can be extracted into a utility function that accepts the pull request and context as parameters and returns a boolean value if the commit is older than the minimum build date.

addressed in latest commit.

nlok5923

also moved the build check repetative script to utils

nlok5923 · 2021-04-01T05:06:30Z

lib/checkPullRequestLabels.js

+ *
+ * @param {import('probot').Context} context
+ */
+module.exports.checkStaleBuildLabelRemoval = async function (context) {


added test in latest commit

vojtechjelinek

Thanks! Left a few comments.

vojtechjelinek · 2021-04-01T08:46:09Z

lib/checkPullRequestLabels.js

+ *
+ * @param {import('probot').Context} context
+ */
+module.exports.checkStaleBuildLabelRemoval = async function (context) {


Suggested change

module.exports.checkStaleBuildLabelRemoval = async function (context) {

module.exports.checkStaleBuildLabelRemoved = async function (context) {

changed function name in latest commit.

vojtechjelinek · 2021-04-01T08:46:25Z

lib/checkPullRequestLabels.js

+    /*const {data: lastCommit} = await context.github.repos.getCommit(
+      context.repo({
+        ref: pullRequest.head.sha
+      })
+    );*/


Do not keep code that is commented.

yes removed commented code.

vojtechjelinek · 2021-04-01T08:47:54Z

lib/utils.js

+const checkBuild = async (pullRequest, context) => {
+    const {data: lastCommit} = await context.github.repos.getCommit(
+      context.repo({
+        ref: pullRequest.head.sha
+      })
+    );
+
+    const lastCommitDate = new Date(lastCommit.commit.author.date);
+    // Commit is older than 2 days
+    return (MIN_BUILD_DATE > lastCommitDate)
+}


Suggested change

const checkBuild = async (pullRequest, context) => {

const {data: lastCommit} = await context.github.repos.getCommit(

context.repo({

ref: pullRequest.head.sha

})

);

const lastCommitDate = new Date(lastCommit.commit.author.date);

// Commit is older than 2 days

return (MIN_BUILD_DATE > lastCommitDate)

}

const checkPrIsNotStale = async (pullRequest, context) => {

const {data: lastCommit} = await context.github.repos.getCommit(

context.repo({

ref: pullRequest.head.sha

})

);

const lastCommitDate = new Date(lastCommit.commit.author.date);

// Commit is older than 2 days

return (MIN_BUILD_DATE > lastCommitDate)

}

vojtechjelinek · 2021-04-01T08:48:42Z

lib/checkPullRequestLabels.js

+      })
+    );*/
+
+    // const lastCommitDate = new Date(lastCommit.commit.author.date);


Remove this.

vojtechjelinek · 2021-04-01T08:50:05Z

lib/checkPullRequestLabels.js

+    );*/
+
+    // const lastCommitDate = new Date(lastCommit.commit.author.date);
+    // Commit is older than 2 days


This comment can easily become stale, since the number of days depends on the checkBuild method.

vojtechjelinek · 2021-04-01T08:50:39Z

lib/checkPullRequestLabels.js

+    if (checkBuild(pullRequest, context)) {
+      const commentParams = context.issue({
+        body:
+        'Hi @' + user.login + ', the build of this pr is ' +


Suggested change

'Hi @' + user.login + ', the build of this pr is ' +

'Hi @' + user.login + ', the build of this PR is ' +

addressed in latest commit.

jameesjohn

Thanks, took another pass.

You can run the tests locally to be sure your changes didn't break a previous implementation.

jameesjohn · 2021-04-04T12:21:56Z

lib/staleBuildChecks.js

  // Commit is older than 2 days, this can happen when 'synchronize' event
  // got triggered as a result of a merge to the main branch. In this case
  // we don't want to remove the label.
-  if (utils.MIN_BUILD_DATE > lastCommitDate) {
+  if (utils.checkBuild(pullRequest, context)) {


There's no utils.checkBuild function.

Extremely sorry sir for such mistake fixing it in latest commit.

jameesjohn · 2021-04-04T12:22:12Z

lib/staleBuildChecks.js

-    const lastCommitDate = new Date(lastCommit.commit.author.date);
-    // Commit is older than 2 days
-    if (utils.MIN_BUILD_DATE > lastCommitDate) {
+    if (utils.checkBuild(pullRequest, context)) {


There's no utils.checkBuild function.

Extremely sorry sir for such mistake fixing it in latest commit.

jameesjohn · 2021-05-29T13:40:09Z

spec/checkPullRequestLabelsSpec.js

@@ -22,7 +22,10 @@ const checkCriticalPullRequestModule =
  require('../lib/checkCriticalPullRequest');
 const checkPullRequestTemplateModule =
  require('../lib/checkPullRequestTemplate');
+const checkStaleBuildLabelRemovedModule =


This module has been imported line 21

jameesjohn · 2021-06-12T13:02:53Z

spec/checkPullRequestLabelsSpec.js

+        getCommit: jasmine.createSpy('getCommit').and.resolveTo({
+          commit: {
+            author: {
+              date: '2021-04-12T18:33:45Z'
+            }
+          }
+        })


Suggested change

getCommit: jasmine.createSpy('getCommit').and.resolveTo({

commit: {

author: {

date: '2021-04-12T18:33:45Z'

}

}

})

getCommit: jasmine.createSpy('getCommit').and.resolveTo({

data: {

commit: {

author: {

date: '2021-04-12T18:33:45Z'

}

}

}

})

Also, move this spy to the different describe blocks that need it so that you will be able to update the data appropriately

jameesjohn · 2021-06-12T13:18:17Z

spec/checkPullRequestLabelsSpec.js

+    beforeEach(async () => {
+      payloadData.payload.action = 'unlabeled';
+      payloadData.payload.label = label;
+      payloadData.payload.sender.login = 'seanlip';
+      spyOn(
+        checkStaleBuildLabelRemovedModule, 'checkStaleBuildLabelRemoved'
+      ).and.callThrough();
+      await robot.receive(payloadData);


Mock getCommit here to signify that the pull request has been updated recently.
You can set the commit date to be (new Date).toISOString(), which will make sure that the commit date will always be more than the minimum date.

Do this in every other place that requires it.

addressed in latest commit.

oppiabot · 2021-06-12T14:36:02Z

Unassigning @jameesjohn since the review is done.

nlok5923 · 2021-06-14T12:14:32Z

@jameesjohn can you please provide a little review over the latest commit as my current changes do not reducing the no of failed tests. also please can you comment on why this pr is experiencing merge conflicts.

updating branch

nlok5923 · 2021-06-16T07:00:24Z

@jameesjohn please review this pr i had tried to bring all the changes suggested by you but still i am receiving test failing you can see log here. one more thing is due to recent merge the specs are not running while npm test

jameesjohn · 2021-06-22T09:07:26Z

spec/checkPullRequestLabelsSpec.js

+        getCommit: jasmine.createSpy('getCommit').and.resolveTo({
+          data: {
+            commit: {
+              author: {
+                date: '2021-04-12T18:33:45Z'
+              }
+            }
+          }
+        })


This should be removed from here and only added to places where it is required.

jameesjohn · 2021-06-22T09:08:26Z

spec/checkPullRequestLabelsSpec.js

+      await robot.receive(payloadData);
+      github = {
+        issues: {
+          createComment: jasmine.createSpy('createComment').and.returnValue({}),
+          addAssignees: jasmine.createSpy('addAssignees').and.resolveTo({}),
+          removeLabel: jasmine.createSpy('removeLabel').and.resolveTo({}),
+          addLabels: jasmine.createSpy('addLabels').and.resolveTo({}),
+        },
+        repos: {
+          getCommit: jasmine.createSpy('getCommit').and.resolveTo({
+            lastCommit: {
+              commit: {
+                author: {
+                  date: (new Date).toISOString()
+                }
+              }
+            }
+          })
+        }
+      };
+    });


Suggested change

await robot.receive(payloadData);

github = {

issues: {

createComment: jasmine.createSpy('createComment').and.returnValue({}),

addAssignees: jasmine.createSpy('addAssignees').and.resolveTo({}),

removeLabel: jasmine.createSpy('removeLabel').and.resolveTo({}),

addLabels: jasmine.createSpy('addLabels').and.resolveTo({}),

},

repos: {

getCommit: jasmine.createSpy('getCommit').and.resolveTo({

lastCommit: {

commit: {

author: {

date: (new Date).toISOString()

}

}

}

})

}

};

});

github.repos.getCommit = jasmine.createSpy('getCommit').and.resolveTo({

data: {

commit: {

author: {

date: '2021-04-12T18:33:45Z'

}

}

}

});

await robot.receive(payloadData);

addressed in latest commit

jameesjohn · 2021-06-22T09:09:17Z

spec/checkPullRequestLabelsSpec.js

+    it('check if pr is stale', () => {
+      expect(github.repos.getCommit).toHaveBeenCalled();
+      expect(github.repos.getCommit).toHaveBeenCalledWith({
+        ref: payloadData.payload.pull_request.head.sha


Suggested change

ref: payloadData.payload.pull_request.head.sha

owner: 'oppia',

repo: 'oppia',

ref: payloadData.payload.pull_request.head.sha

jameesjohn · 2021-06-22T09:23:42Z

spec/checkPullRequestLabelsSpec.js

+      await robot.receive(payloadData);
+      github = {
+        issues: {
+          createComment: jasmine.createSpy('createComment').and.returnValue({}),
+          addAssignees: jasmine.createSpy('addAssignees').and.resolveTo({}),
+          removeLabel: jasmine.createSpy('removeLabel').and.resolveTo({}),
+          addLabels: jasmine.createSpy('addLabels').and.resolveTo({}),
+        },
+        repos: {
+          getCommit: jasmine.createSpy('getCommit').and.resolveTo({
+            lastCommit: {
+              commit: {
+                author: {
+                  date: '2021-04-12T18:33:45Z'
+                }
+              }
+            }
+          })
+        }
+      };


Suggested change

await robot.receive(payloadData);

github = {

issues: {

createComment: jasmine.createSpy('createComment').and.returnValue({}),

addAssignees: jasmine.createSpy('addAssignees').and.resolveTo({}),

removeLabel: jasmine.createSpy('removeLabel').and.resolveTo({}),

addLabels: jasmine.createSpy('addLabels').and.resolveTo({}),

},

repos: {

getCommit: jasmine.createSpy('getCommit').and.resolveTo({

lastCommit: {

commit: {

author: {

date: '2021-04-12T18:33:45Z'

}

}

}

})

}

};

github.repos.getCommit = jasmine.createSpy('getCommit').and.resolveTo({

data: {

commit: {

author: {

date: (new Date).toString()

}

}

}

});

jameesjohn · 2021-06-22T09:24:18Z

spec/checkPullRequestLabelsSpec.js

+      expect(github.repos.getCommit).toHaveBeenCalledWith({
+        ref: payloadData.payload.pull_request.head.sha
+      });


Suggested change

expect(github.repos.getCommit).toHaveBeenCalledWith({

ref: payloadData.payload.pull_request.head.sha

});

expect(github.repos.getCommit).toHaveBeenCalledWith({

repo: 'oppia',

owner: 'oppia',

ref: payloadData.payload.pull_request.head.sha

});

oppiabot · 2021-06-22T09:27:57Z

Unassigning @jameesjohn since the review is done.

oppiabot · 2021-06-22T09:28:00Z

Hi @nlok5923, it looks like some changes were requested on this pull request by @jameesjohn. PTAL. Thanks!

nlok5923 · 2021-06-22T10:53:47Z

@jameesjohn thanks for guidance now all checks are passing.

re add stale Build label is if someone remove it

7ee56ed

nlok5923 changed the title ~~Fix: add stale build label if if it removed before updating branch~~ Fix: add stale build label if it is removed before updating branch Mar 28, 2021

vojtechjelinek assigned vojtechjelinek and jameesjohn Mar 28, 2021

vojtechjelinek requested review from jameesjohn and vojtechjelinek and removed request for jameesjohn March 28, 2021 16:42

vojtechjelinek requested changes Mar 28, 2021

View reviewed changes

vojtechjelinek removed their assignment Mar 28, 2021

nlok5923 added 3 commits March 28, 2021 22:27

changes addressed

daf0a43

requested changes addressed

7915838

addressed

7f627e3

nlok5923 requested a review from vojtechjelinek March 28, 2021 17:46

jameesjohn suggested changes Mar 28, 2021

View reviewed changes

addressed requested changes

50b76bf

nlok5923 requested a review from jameesjohn March 29, 2021 02:54

jameesjohn suggested changes Mar 30, 2021

View reviewed changes

Added test and moved function to util

e27e96c

nlok5923 commented Apr 1, 2021

View reviewed changes

vojtechjelinek requested changes Apr 1, 2021

View reviewed changes

addressed requested changes

82aa6c9

nlok5923 requested review from vojtechjelinek and jameesjohn April 1, 2021 10:03

chore changes

faf7457

jameesjohn suggested changes Apr 4, 2021

View reviewed changes

jameesjohn removed their assignment Apr 4, 2021

nlok5923 added 2 commits June 7, 2021 22:51

chnages

663a30e

fix merge conflicts

d49a225

oppiabot bot assigned nlok5923 Jun 8, 2021

jameesjohn suggested changes Jun 12, 2021

View reviewed changes

oppiabot bot unassigned jameesjohn Jun 12, 2021

nlok5923 added 2 commits June 12, 2021 20:44

addressing changes

7c9b3fb

addressing changes

166aeb0

nlok5923 added 4 commits June 15, 2021 15:42

adding changes

fb439f6

conflicts resolved

2fd27aa

resolving conflicts

5a1095a

Merge branch 'oppia-master' into check-build-label

72e2f43

updating branch

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Jun 15, 2021

working on test

abec6a5

nlok5923 assigned jameesjohn and unassigned nlok5923 Jun 16, 2021

jameesjohn suggested changes Jun 22, 2021

View reviewed changes

oppiabot bot unassigned jameesjohn Jun 22, 2021

oppiabot bot assigned nlok5923 Jun 22, 2021

changes reverted and test corrected

836b150

nlok5923 requested a review from jameesjohn June 22, 2021 14:13

nlok5923 assigned jameesjohn and unassigned nlok5923 Jun 22, 2021

jameesjohn approved these changes Jun 29, 2021

View reviewed changes

jameesjohn merged commit eea2542 into oppia:master Jun 29, 2021

	' stale please do not remove ' + [OLD_BUILD_LABEL] +
	' stale please do not remove \'' + OLD_BUILD_LABEL + '\''

		* This function checks and prevent the removal of critical label
		* by unauthorised users.

	const {data: lastCommit} = await context.github.repos.getCommit(
	context.repo({
	ref: pullRequest.head.sha
	})
	);

	const lastCommitDate = new Date(lastCommit.commit.author.date);
	// Commit is older than 2 days
	if (utils.MIN_BUILD_DATE > lastCommitDate) {

	module.exports.checkBuildLabelRemoval = async function (context) {
	module.exports.checkStaleBuildLabelRemoval = async function (context) {

	checkPullRequestLabelsModule.checkBuildLabelRemoval(context)
	checkPullRequestLabelsModule.checkStaleBuildLabelRemoval(context)

	'Hi @' + user.login + ', the build of this pr is ' +
	'Hi @' + user.login + ', the build of this PR is ' +

Fix: add stale build label if it is removed before updating branch #235

Fix: add stale build label if it is removed before updating branch #235

Conversation

nlok5923 commented Mar 28, 2021 • edited Loading

Explanation

Checklist

nlok5923 commented Mar 28, 2021

nlok5923 commented Mar 28, 2021

vojtechjelinek left a 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

jameesjohn left a 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

nlok5923 commented Mar 30, 2021

jameesjohn left a 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

nlok5923 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechjelinek left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameesjohn left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Jun 12, 2021

nlok5923 commented Jun 14, 2021

nlok5923 commented Jun 16, 2021 • edited Loading

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

oppiabot bot commented Jun 22, 2021

oppiabot bot commented Jun 22, 2021

nlok5923 commented Jun 22, 2021

nlok5923 commented Mar 28, 2021 •

edited

Loading

nlok5923 commented Jun 16, 2021 •

edited

Loading