Fix #5938: Lint checks will run only for the files changed by the user #6033

lilithxxx · 2018-12-27T17:47:58Z

Explanation

Previously the pre_push_hook tested against the remote branch of our local repository (i.e local_repo:develop) but now is changed to test against the remote branch of Oppia's repository (i.e oppia:develop). This will prevent the linter to check the files touched by a foreign commit.

This requires user to have their upstream set.

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR follows the style guide.
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue.

codecov-io · 2018-12-27T19:11:50Z

Codecov Report

Merging #6033 into develop will decrease coverage by 0.05%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #6033      +/-   ##
===========================================
- Coverage    45.27%   45.21%   -0.06%     
===========================================
  Files          521      523       +2     
  Lines        30688    30722      +34     
  Branches      4597     4605       +8     
===========================================
- Hits         13894    13892       -2     
- Misses       16794    16830      +36

Impacted Files	Coverage Δ
...dev/head/pages/exploration_editor/RouterService.js	`17.5% <0%> (-5.31%)`	⬇️
...head/pages/exploration_editor/ExplorationEditor.js	`5.12% <0%> (-0.11%)`	⬇️
...es/exploration_editor/EditorNavigationDirective.js	`2.08% <0%> (-0.1%)`	⬇️
...ges/exploration_editor/settings_tab/SettingsTab.js	`0.62% <0%> (ø)`	⬆️
...ad/pages/skill_editor/SkillEditorRoutingService.js	`4% <0%> (ø)`	⬆️
...s/exploration_editor/ExplorationWarningsService.js	`11.34% <0%> (ø)`	⬆️
...loration_editor/EditorNavbarBreadcrumbDirective.js	`5.55% <0%> (ø)`	⬆️
...ad/pages/topic_editor/TopicEditorRoutingService.js	`3.33% <0%> (ø)`	⬆️
...ditor/CollectionEditorNavbarBreadcrumbDirective.js	`9.09% <0%> (ø)`	⬆️
...r/editor_tab/UnresolvedAnswersOverviewDirective.js	`1.13% <0%> (ø)`	⬆️
... and 6 more

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 59260be...8bb2609. Read the comment docs.

apb7 · 2018-12-30T16:44:36Z

scripts/install_third_party.sh

+if [ ! -d "$TOOLS_DIR/GitPython" ]; then
+  echo Installing GitPython
+
+  pip install gitpython --target="$TOOLS_DIR/GitPython"


A couple of points here:

Can we fix the version of gitpython we use here?

Can we use curl instead of pip?

Also, the tests seem to be failing on Travis due to import error (see here).

apb7 · 2018-12-30T18:42:03Z

@lilithxxx, the import error in the tests is due to the fact that git module has not been added to system paths. See around this line on how to insert the required path: https://github.com/oppia/oppia/blob/develop/scripts/pre_commit_linter.py#L273

lilithxxx · 2018-12-31T17:19:07Z

Hi @apb7 I changed the code to use subprocess instead of gitpython. PTAL

apb7

Hi @lilithxxx, I think using GitPython would be quite better than using subprocess -- it will streamline the workflow here. Thanks!

lilithxxx · 2019-01-01T19:38:01Z

@apb7 Made some changes. PTAL

apb7 · 2019-01-02T05:25:59Z

@lilithxxx, there are still import errors for the package. ptal! Thanks.

apb7 · 2019-01-02T06:40:16Z

@lilithxxx, I looked into the import error with GitPython. This SO post is useful here -- you need gitdb and smmap as well in the path. Also, I think that adding the paths to these modules in pre_commit_linter.py itself will be sufficient.

apb7 · 2019-01-02T19:45:00Z

scripts/install_third_party.sh

@@ -148,6 +148,24 @@ if [ ! -d "$TOOLS_DIR/isort-4.2.15" ]; then
  rm isort-4.2.15.tar.gz
 fi

+# Install GitPython and its dependencies.


A couple of points here -- make this as three separate echos instead of one Installing GitPython and its dependencies so that even if any one of them is already installed, we skip that.

apb7 · 2019-01-02T19:46:35Z

scripts/pre_commit_linter.py

@@ -292,6 +292,9 @@
    os.path.join(_PARENT_DIR, 'oppia_tools', 'pylint-quotes-0.1.9'),
    os.path.join(_PARENT_DIR, 'oppia_tools', 'selenium-2.53.2'),
    os.path.join(_PARENT_DIR, 'oppia_tools', 'PIL-1.1.7'),
+    os.path.join(_PARENT_DIR, 'oppia_tools', 'GitPython-2.1.11'),


Place smmap first then gitdb and then GitPython (in the order of the dependence relation).

So sorry -- done now

apb7 · 2019-01-02T19:47:38Z

scripts/pre_push_hook.py


+# Add GitPython to sys.path, then only it can be imported.


Change this to Insert GitPython to sys.path so that it can be imported.

apb7 · 2019-01-02T19:49:42Z

scripts/pre_push_hook.py

+            remote_num += 1
+            remote_name = remote.name
+
+    if remote_num == 0:


You can remove the equality check, 0 will evaluate to false.

I don't understand this -- We are checking if there is an upstream set or not. Can you please come again? Thanks

My bad, make it if not remote_num.

apb7 · 2019-01-02T19:51:29Z

scripts/pre_push_hook.py

-    for branch, sha1, remote_sha1 in zip(branches, hashes, remote_hashes):
-        # Git reports the following for an empty / non existing branch
-        # sha1: '0000000000000000000000000000000000000000'.
-        if set(remote_sha1) != {'0'}:


Why don't we require this part, that is, the removed portion of the code now?

Here it was comparing the changes of the local branch with the local branch itself if the branch is not empty and compared with origin if the branch was empty(i.e had no changes in it)
Now, we are making it compare to upstream at all time.

lilithxxx · 2019-01-02T20:26:27Z

@apb7 Made the changes. PTAL

apb7 · 2019-01-02T20:34:52Z

scripts/pre_push_hook.py

@@ -94,6 +103,33 @@ def _start_subprocess_for_result(cmd):
    return out, err


+def _get_remote_name():
+    """Get the remote name of the local repository.


I think you're missing a new line here.

apb7 · 2019-01-03T09:23:47Z

scripts/pre_push_hook.py


+# Insert GitPython to sys.path so that it can be imported.
+_PARENT_DIR = os.path.abspath(os.path.join(os.getcwd(), os.pardir))
+GIT_PYTHON_PATH = os.path.join(_PARENT_DIR, 'oppia_tools', 'GitPython-2.1.11')


@lilithxxx, the build is failing due to import error. I think you'll have to add paths to both the dependencies here as well.

apb7

LGTM, thanks @lilithxxx! Tested it by pulling this branch and making several merge commits -- the functionality seems to be working as expected.

apb7 · 2019-01-03T11:40:20Z

@nithusha21, if you're around, can you please give this a try before we merge this? Thanks!

nithusha21 · 2019-01-03T16:23:55Z

Tested locally, seems to be working fine! Thanks for the fix @lilithxxx :)

Rishav Chakraborty added 2 commits December 27, 2018 23:01

Test against upstream

1a242f3

Merge branch 'develop' into lint-checks

628cf13

lilithxxx requested review from apb7, BenHenning, nithusha21 and seanlip as code owners December 27, 2018 17:47

Rishav Chakraborty added 8 commits December 29, 2018 01:04

Removed hard-coded upstream

2b4aa19

Merge branch 'develop' into lint-checks

9b52b60

Only checks with upstream/develop

52b65de

Deleted log file

11269fa

Added gitpython to third party

04fd285

Test1

ddb7962

Removed remote_sha1

5b44137

Removed test print statements

6474d48

apb7 suggested changes Dec 30, 2018

View reviewed changes

Curl instead of pip

82ca125

DubeySandeep assigned apb7 Jan 1, 2019

apb7 reviewed Jan 1, 2019

View reviewed changes

apb7 assigned lilithxxx and unassigned apb7 Jan 1, 2019

Rishav Chakraborty added 3 commits January 2, 2019 00:58

Fixed import of git

9bc51b6

Merge branch 'develop' into lint-checks

59df35b

Fixed import error

41b8aeb

lilithxxx force-pushed the lint-checks branch from ab43c71 to 41b8aeb Compare January 1, 2019 19:36

Rishav Chakraborty added 2 commits January 3, 2019 00:03

Install gitpython requirements

5ea3a76

Add gitpython path to system path

9b80e62

apb7 suggested changes Jan 2, 2019

View reviewed changes

Change order of install

31f235f

Changed order

d39296b

apb7 reviewed Jan 2, 2019

View reviewed changes

Add new line

5ad6c34

apb7 reviewed Jan 3, 2019

View reviewed changes

Add gitpython's requirements path in pre_push

8bb2609

apb7 approved these changes Jan 3, 2019

View reviewed changes

apb7 assigned nithusha21 and unassigned lilithxxx Jan 3, 2019

nithusha21 merged commit 52a859f into oppia:develop Jan 3, 2019

lilithxxx deleted the lint-checks branch January 3, 2019 17:05

This was referenced Jan 14, 2019

Remove third-party libs added in PR #6033 and try using subprocess #6104

Closed

The pre-push checks do not run for branches created off oppia/oppia.git #6105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #5938: Lint checks will run only for the files changed by the user #6033

Fix #5938: Lint checks will run only for the files changed by the user #6033

lilithxxx commented Dec 27, 2018 •

edited

Loading

codecov-io commented Dec 27, 2018 •

edited

Loading

apb7 Dec 30, 2018

lilithxxx Dec 30, 2018

apb7 commented Dec 30, 2018

lilithxxx commented Dec 31, 2018

apb7 left a comment

lilithxxx commented Jan 1, 2019

apb7 commented Jan 2, 2019

apb7 commented Jan 2, 2019

apb7 Jan 2, 2019

lilithxxx Jan 2, 2019

apb7 Jan 2, 2019

lilithxxx Jan 2, 2019

apb7 Jan 2, 2019

lilithxxx Jan 2, 2019

apb7 Jan 2, 2019

lilithxxx Jan 2, 2019

apb7 Jan 2, 2019

lilithxxx Jan 2, 2019

apb7 Jan 2, 2019

lilithxxx Jan 2, 2019

apb7 Jan 2, 2019

lilithxxx Jan 2, 2019 •

edited

Loading

lilithxxx commented Jan 2, 2019

apb7 Jan 2, 2019

lilithxxx Jan 2, 2019

apb7 Jan 3, 2019

lilithxxx Jan 3, 2019

apb7 left a comment

apb7 commented Jan 3, 2019

nithusha21 commented Jan 3, 2019

Fix #5938: Lint checks will run only for the files changed by the user #6033

Fix #5938: Lint checks will run only for the files changed by the user #6033

Conversation

lilithxxx commented Dec 27, 2018 • edited Loading

Explanation

Checklist

codecov-io commented Dec 27, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apb7 commented Dec 30, 2018

lilithxxx commented Dec 31, 2018

apb7 left a comment

Choose a reason for hiding this comment

lilithxxx commented Jan 1, 2019

apb7 commented Jan 2, 2019

apb7 commented Jan 2, 2019

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

lilithxxx Jan 2, 2019 • edited Loading

Choose a reason for hiding this comment

lilithxxx commented Jan 2, 2019

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

apb7 left a comment

Choose a reason for hiding this comment

apb7 commented Jan 3, 2019

nithusha21 commented Jan 3, 2019

lilithxxx commented Dec 27, 2018 •

edited

Loading

codecov-io commented Dec 27, 2018 •

edited

Loading

lilithxxx Jan 2, 2019 •

edited

Loading