Fix #50: Check pull request for job files #78

jameesjohn · 2020-06-02T12:42:47Z

Explanation

This PR checks for modification of job files and the job registry in a pull request.

This PR has been tested at jameesjohn/comment-on-pr#31, jameesjohn/comment-on-pr#32

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.

ankita240796

Hi @jameesjohn, took a pass, PTAL. Thanks!

Also, update the PR title and description to include Fixes #50.

ankita240796 · 2020-06-05T20:38:55Z

lib/checkPullRequestJob.js

+  const REGISTRY_FILENAME = 'jobs_registry.py';
+  const JOB_FILE_FIX = '_jobs_';


Define these outside the function.

ankita240796 · 2020-06-05T20:39:21Z

lib/checkPullRequestJob.js

+  const REGISTRY_FILENAME = 'jobs_registry.py';
+  const JOB_FILE_FIX = '_jobs_';


What does FIX represent here?

I wasn't sure of what to name since this isn't a prefix or suffix.

ankita240796 · 2020-06-05T20:40:20Z

lib/checkPullRequestJob.js

+    const commentBody = 'Hi @' + SERVER_JOBS_ADMIN + ', PTAL at this PR, ' +
+      'it adds/modifies a server job. Also @' + prAuthor + ', please ' +
+      'endeavour to fill the ' + serverJobsForm + ' for the new job ' +
+      'to get tested on the test server. Thanks!';


to get tested on the test server -> to be tested on the backup server. This PR can be merged only after the test is successful.

ankita240796 · 2020-06-05T20:42:15Z

lib/checkPullRequestJob.js

+      'https://goo.gl/forms/XIj00RJ2h5L55XzU2');
+    const commentBody = 'Hi @' + SERVER_JOBS_ADMIN + ', PTAL at this PR, ' +
+      'it adds/modifies a server job. Also @' + prAuthor + ', please ' +
+      'endeavour to fill the ' + serverJobsForm + ' for the new job ' +


endeavour to fill the -> fill the.

Also, provide a link to the wiki page with instructions for new jobs: https://github.com/oppia/oppia/wiki/Running-jobs-in-production#submitting-a-pr-with-a-new-job.

Also, can we mark such PRs as unmergeable until approved by Server jobs admin?

\cc @seanlip @nithusha21 @DubeySandeep

I think what we should do here is to assign the pr to the server jobs admin.

That makes sense. Let us do that.

ankita240796 · 2020-06-05T20:44:12Z

lib/checkPullRequestJob.js

+      body: commentBody,
+    });
+
+    await context.github.issues.createComment(commentParams);


We should also add the critical label to the PR.

ankita240796 · 2020-06-05T20:45:34Z

lib/checkPullRequestJob.js

+/**
+ * @param {string} filename
+ */
+const isJobFile = (filename) => {


What if a job file is being modified but no new job is introduced? There can be a case when an issue is being fixed or some variables are being renamed. We only need the test procedure for new jobs.

Further, we need to have a check which ensures that whenever a new job is added, the name of the job is added to jobs_registry file.

We may not be able to check the exact content of the job_registry file. What we can do is to ensure that a new job file is accompanied with a change in the job_registry.

We may not be able to check the exact content of the job_registry file. What we can do is to ensure that a new job file is accompanied with a change in the job_registry.

I've been able to do this.

…-pr-job

jameesjohn · 2020-06-09T14:23:46Z

PTAL @ankita240796.
I've addressed all comments. The implementation here is significantly different from the initial one.

Thanks.

jameesjohn · 2020-06-09T14:25:10Z

I also got some inputs from @seanlip regarding the oppiabot message.

ankita240796

Hi @jameesjohn, took a pass, PTAL. Thanks!

ankita240796 · 2020-06-09T19:22:15Z

constants.js

  return checksWhitelist;
 };
-


Done.
My formatter added these.

No issues, thanks for fixing! I think a line is still being removed. We should have a new line at file end,right? Let us revert that please.

There is a new line at the end of the file.

Ah ok I see, there was an extra line.

ankita240796 · 2020-06-09T19:22:54Z

lib/checkPullRequestJob.js

@@ -0,0 +1,210 @@
+const {SERVER_JOBS_ADMIN} = require('../userWhitelist.json');


Space after brackets.

Missed this one.

ankita240796 · 2020-06-09T19:23:48Z

package.json

@@ -44,7 +44,8 @@
  "nodemonConfig": {
    "exec": "npm start",
    "watch": [
-      ".env"
+      ".env",
+      "."


Why this change?

I noticed that nodemon was not restarting when the implementation code changed so adding . tells nodemon to watch the directory for the files which are expected (.js, .ts, .json) files.

Ok, Thanks for doing this!

ankita240796 · 2020-06-09T19:26:03Z

constants.js

@@ -22,25 +23,16 @@ const checksWhitelist = {
    [labelEvent]: [],
    [synchronizeEvent]: [],
    [closeEvent]: [],
-    [editEvent]: []
+    [editEvent]: [],


We should not have terminating , in an array list in JavaScript. Revert here and below.

ankita240796 · 2020-06-09T19:29:00Z

lib/checkPullRequestJob.js

+  // Filename is the relative path to the file, we need to
+  // get the exact file name.
+  const substrings = filename.split('/');
+  const actualFileNameWithExtension = substrings[substrings.length - 1];
+  const extensionIndex = actualFileNameWithExtension.indexOf('.py');
+
+  return actualFileNameWithExtension.substring(0, extensionIndex);


I am a bit confused on what this function is doing, are you obtaining the name of a job being added or the name of the file without any dir paths in it? Can you add some examples in comments and update the name of the function accordingly if it is being used to obtain file name without extensions.

It is getting the name of the job from the file name and the name of the job does not include the file extension.

Can you add an example in the comments?

ankita240796 · 2020-06-09T19:37:26Z

lib/checkPullRequestJob.js

+  // This function will never be called when there are no job files.
+  if (newJobFiles.length === 1) {
+    const serverAdminPing = 'Hi @' + SERVER_JOBS_ADMIN + ', PTAL at this PR, ' +
+      'it adds a new server job.' + jobNameString;


server job -> one off job

ankita240796 · 2020-06-09T19:39:14Z

lib/checkPullRequestJob.js

+    const prAuthorPing = 'Also @' + prAuthor + registryReminder +
+    'please make sure to fill in the ' + serverJobsForm +
+    ' for the new job to be tested on the backup server. ' +
+    'This PR can be merged only after the test is successful.';


Add a link to the guide on jobs: https://github.com/oppia/oppia/wiki/Running-jobs-in-production#submitting-a-pr-with-a-new-job.

ankita240796 · 2020-06-09T19:39:19Z

lib/checkPullRequestJob.js

+    const prAuthorPing = 'Also @' + prAuthor + registryReminder +
+    'please make sure to fill in the ' + serverJobsForm +
+    ' for the new jobs  to be tested on the backup server.' +
+    ' This PR can be merged only after the test is successful.';


ankita240796 · 2020-06-09T19:40:55Z

lib/checkPullRequestJob.js

+  if (hasNotAddedFileToRegistry) {
+    const fileText = newJobFiles.length > 1 ? 'files' : 'file';
+    registryReminder +=
+      'endeavour to add the new job ' + fileText + ' to the job registry and ';


Please add the new job...

ankita240796 · 2020-06-09T19:41:33Z

spec/checkPullRequestJobSpec.js

+      'https://github.com/oppia/oppia/blob/67fb4a973b318882af3b5a894130e110d7e9833c/core/domain/exp_jobs_oppiabot_off.py',
+    raw_url:
+      'https://github.com/oppia/oppia/raw/67fb4a973b318882af3b5a894130e110d7e9833c/core/domain/exp_jobs_oppiabot_off.py',
+    contents_url:


Can we use this to obtain the content of the file?

Probably
The changes in the file are in the patch

Ok, so we can use patch for finding new jobs being added in an existing file, right?

ankita240796

Took a pass, PTAL @jameesjohn. Thanks!

ankita240796 · 2020-06-10T10:41:16Z

lib/checkPullRequestJob.js

@@ -0,0 +1,210 @@
+const {SERVER_JOBS_ADMIN} = require('../userWhitelist.json');


Missed this one.

ankita240796 · 2020-06-10T10:42:05Z

constants.js

@@ -22,25 +23,16 @@ const checksWhitelist = {
    [labelEvent]: [],
    [synchronizeEvent]: [],
    [closeEvent]: [],
-    [editEvent]: []
+    [editEvent]: [],


ankita240796 · 2020-06-10T10:42:19Z

constants.js

  return checksWhitelist;
 };
-


ankita240796 · 2020-06-10T10:43:08Z

lib/checkPullRequestJob.js

+  // Filename is the relative path to the file, we need to
+  // get the exact file name.
+  const substrings = filename.split('/');
+  const actualFileNameWithExtension = substrings[substrings.length - 1];
+  const extensionIndex = actualFileNameWithExtension.indexOf('.py');
+
+  return actualFileNameWithExtension.substring(0, extensionIndex);


Can you add an example in the comments?

ankita240796 · 2020-06-10T10:43:32Z

lib/checkPullRequestJob.js

+const doesRegistryContainFile = (filename, files) => {
+  const jobName = getJobNameFromFileName(filename);
+
+  const registryFile = files.find(({filename}) => {


Ok, Thanks for explaining.

ankita240796 · 2020-06-10T10:46:32Z

spec/checkPullRequestJobSpec.js

+    });
+  });
+
+  describe('When pull request creates multiple job files', () => {


ankita240796 · 2020-06-10T10:46:52Z

spec/checkPullRequestJobSpec.js

+   */
+  let app;
+
+  const newJobFileObj = {


firstNewJobFileObj

ankita240796 · 2020-06-10T10:47:33Z

spec/checkPullRequestJobSpec.js

+    });
+  });
+
+  describe('When pull request creates a new job file and updates registry', () => {


When pull request creates a new job file and updates registry -> When a new job file is created and registry is updated in a pull request

ankita240796 · 2020-06-10T10:48:33Z

spec/checkPullRequestJobSpec.js

+    });
+  });
+
+  describe('When pull request modifies an existing job file', () => {


We will need to refactor this into two cases:

No new job being added in an existing job file

New job being added in an existing job file

ankita240796 · 2020-06-10T10:48:51Z

spec/checkPullRequestJobSpec.js

+    });
+  });
+
+  describe('when pull request does not modify job file', () => {


When no job file is modified in a pull request

ankita240796 · 2020-06-10T10:49:54Z

Please update the PR title and description to include Fixes #50.

ankita240796

Quite close, a few more comments @jameesjohn, PTAL. Thanks!

ankita240796 · 2020-06-10T15:36:11Z

constants.js

    [closeEvent]: [allMergeConflictCheck],
    [editEvent]: [wipCheck]
-  }
+  },


Please remove the comma. here as well.

ankita240796 · 2020-06-10T15:36:58Z

constants.js

  return checksWhitelist;
 };
-


No issues, thanks for fixing! I think a line is still being removed. We should have a new line at file end,right? Let us revert that please.

ankita240796 · 2020-06-10T15:56:58Z

lib/checkPullRequestJob.js

+   * @type {import('probot').Octokit.PullsGetResponse}
+   */
+  const pullRequest = context.payload.pull_request;
+  // Using 100 here since that is the maximum files per


100 is used here....

ankita240796 · 2020-06-10T15:57:08Z

lib/checkPullRequestJob.js

+   */
+  const pullRequest = context.payload.pull_request;
+  // Using 100 here since that is the maximum files per
+  // request that can be gotten from the API.


gotten -> obtained

ankita240796 · 2020-06-10T16:08:10Z

lib/checkPullRequestJob.js

+  let registryReminder = ', ';
+
+  const hasNotAddedFileToRegistry = newJobFiles.some(
+    (file) => !doesRegistryContainFile(file.filename, changedFiles)


This should actually be checking that registry contains the job name and not only the filename. If you check the registry file, it has jobs listed in the format filename.jobname. So, I would suggest have a function which returns a list of new jobs which are added irrespective of whether it is in a new file or an existing file and then use that list here.

Done.
The function is checking the patch, hence it will only get the new job since that is what we want.

ankita240796 · 2020-06-10T16:09:40Z

lib/checkPullRequestJob.js

+  const classIdentifier = 'class';
+
+  // A new job class should start with `class `
+  return changesArray.some(change => change.startsWith(classIdentifier));


A better check would be to also ensure that the class name ends with OneOffJob. Obtain the list of all such class names instead of file names. (Example: If you check prod validation jobs, there are classes in the file which are validators and not one off jobs.)

ankita240796

Just a few more minor nits, PTAL @jameesjohn!

ankita240796 · 2020-06-11T14:48:13Z

constants.js

  return checksWhitelist;
 };
-


Ah ok I see, there was an extra line.

ankita240796 · 2020-06-11T14:51:02Z

lib/checkPullRequestJob.js

+
+  // New job definitions should look like:
+  // class OppiabotContributionsOneOffJob(jobs.BaseMapReduceOneOffJobManager):
+  // To get the actual job name, we need to remove the unneeded items.


unneeded items -> class prefix and superclass

ankita240796 · 2020-06-11T14:52:24Z

lib/checkPullRequestJob.js

+ * @param {string} job
+ * @param {import('probot').Octokit.PullsListFilesResponse} files
+ */
+const doesRegistryContainFile = (job, files) => {


Rename to isRegistryUpdated since it is checking two things whether registry is modified and if modified does it include the new job.

ankita240796 · 2020-06-11T14:54:02Z

spec/checkPullRequestJobSpec.js

+let payloadData = JSON.parse(
+  JSON.stringify(require('../fixtures/pullRequestPayload.json'))
+);
+const {SERVER_JOBS_ADMIN} = require('../userWhitelist.json');


I think you missed this.

nishantwrp · 2020-06-12T23:19:00Z

I've tested this functionality. Everything seems fine! But there is no comment when an existing job is modified as suggested in the description of this pull request.

jameesjohn · 2020-06-13T09:02:53Z

Thanks @nishantwrp.

But there is no comment when an existing job is modified as suggested in the description of this pull request.

For existing job files, we are only commenting when the modifications includes a new job being added.

ankita240796

Final comments, PTAL @jameesjohn, Thanks!

ankita240796 · 2020-06-13T15:49:44Z

lib/checkPullRequestJob.js

+  // A new job class should start with class and include OneOffJob in its name.
+  const newJobDefinitions = changesArray.filter(
+    (change) =>
+      change.startsWith(classIdentifier) && change.includes(oneOffJobIdentifier)


Let us have a regex instead of this and grab the name using a match group. Would be more readable.

Done.
Changing to regex helped reveal a bug so it was a good call.
Thanks.

ankita240796 · 2020-06-13T15:50:41Z

lib/checkPullRequestJob.js

+  const serverJobsForm = 'server jobs form'.link(
+    'https://goo.gl/forms/XIj00RJ2h5L55XzU2');
+  const wikiLinkText = (
+    'This PR can be merged only after the test is successful'.link(


test -> test run is successful. Please refer this guide [link to this guide] for details

ankita240796 · 2020-06-13T15:52:04Z

lib/checkPullRequestJob.js

+  if (hasNotAddedFileToRegistry) {
+    const fileText = newJobFiles.length > 1 ? 'files' : 'file';
+    registryReminder +=
+      'please add the new job ' + fileText + ' to the job registry and ';


to the job registry [Link to jobs_registry.py]

…-pr-job

ankita240796

LGTM, Thanks @jameesjohn!

ankita240796 · 2020-06-15T18:09:56Z

lib/checkPullRequestJob.js

+
+  if (hasNotAddedFileToRegistry) {
+    const fileText = newJobFiles.length > 1 ? 'files' : 'file';
+    const jobRegistryLink = 'job registry'.link(


job registry file.

ankita240796 · 2020-06-15T18:10:34Z

lib/checkPullRequestJob.js

+  const serverJobsForm = 'server jobs form'.link(
+    'https://goo.gl/forms/XIj00RJ2h5L55XzU2');
+  const wikiLinkText = (
+    'Please refer to this guide for details'.link(


Can we just link to this guide instead of the complete line.

ankita240796 · 2020-06-15T18:11:29Z

lib/checkPullRequestJob.js

+  const jobRegex = /(?<classDefinition>class\s)(?<jobName>[a-zA-Z]{2,256})(?<jobSuffix>OneOffJob)/;
+
+  const newJobDefinitions = changesArray.filter(change => {
+    const matches = jobRegex.exec(change);
+    return matches !== null;
+  });
+
+  const newJobs = newJobDefinitions.map(definition => {
+    const matches = jobRegex.exec(definition);
+    return matches.groups.jobName + matches.groups.jobSuffix;
+  });


Thanks for doing this!

Yeah...
Nitish's article helped a lot.

@bansalnitish ping :D

jameesjohn · 2020-06-16T07:11:10Z

Hi @ankita240796, this is ready to be merged right?

ankita240796 · 2020-06-17T19:23:41Z

Yes, thanks @jameesjohn! Thanks for doing this so well!

jameesjohn added 4 commits May 28, 2020 17:02

start working on jobs check

52408d1

minor update

9c03544

write tests

4d208c1

sentence changes

602e16c

jameesjohn requested a review from ankita240796 June 2, 2020 12:42

jameesjohn assigned ankita240796 Jun 2, 2020

ankita240796 suggested changes Jun 5, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Jun 5, 2020

jameesjohn added 5 commits June 9, 2020 15:01

Address review comments

d24bacf

update watcher

f82ddaf

Merge branch 'master' of https://github.com/oppia/oppiabot into check…

8ed8f1b

…-pr-job

modifications

7180b6f

address more comments

d609743

jameesjohn requested a review from ankita240796 June 9, 2020 14:23

jameesjohn assigned ankita240796 and unassigned jameesjohn Jun 9, 2020

ankita240796 suggested changes Jun 9, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Jun 9, 2020

address review comments

fef4ae2

jameesjohn requested a review from ankita240796 June 10, 2020 10:11

jameesjohn assigned ankita240796 and unassigned jameesjohn Jun 10, 2020

ankita240796 suggested changes Jun 10, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Jun 10, 2020

ankita240796 suggested changes Jun 10, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Jun 10, 2020

Address more comments

65121b6

ankita240796 suggested changes Jun 11, 2020

View reviewed changes

jameesjohn added 2 commits June 12, 2020 10:16

address reveiw comments

56285fe

also check on PR reopening

253064e

fix tests

658776e

jameesjohn requested a review from ankita240796 June 13, 2020 09:51

jameesjohn assigned ankita240796 and unassigned jameesjohn Jun 13, 2020

ankita240796 suggested changes Jun 13, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Jun 13, 2020

address review comments

ff64a39

jameesjohn requested a review from ankita240796 June 15, 2020 11:54

jameesjohn assigned ankita240796 and unassigned jameesjohn Jun 15, 2020

Merge branch 'master' of https://github.com/oppia/oppiabot into check…

2e16a40

…-pr-job

ankita240796 approved these changes Jun 15, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Jun 15, 2020

address comments

b104989

jameesjohn assigned ankita240796 and unassigned jameesjohn Jun 16, 2020

ankita240796 merged commit d9c7fce into oppia:master Jun 17, 2020

		const REGISTRY_FILENAME = 'jobs_registry.py';
		const JOB_FILE_FIX = '_jobs_';

		@@ -0,0 +1,210 @@
		const {SERVER_JOBS_ADMIN} = require('../userWhitelist.json');

Fix #50: Check pull request for job files #78

Fix #50: Check pull request for job files #78

Conversation

jameesjohn commented Jun 2, 2020

Explanation

Checklist

ankita240796 left a comment • 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

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 commented Jun 9, 2020

jameesjohn commented Jun 9, 2020

ankita240796 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

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

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

ankita240796 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankita240796 commented Jun 10, 2020

ankita240796 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankita240796 left a comment •

edited

Loading