Automatically detect default branch name #179

sean-morris · 2021-05-28T21:55:11Z

Added requirements.txt file
Added venv/ folder to .gitignore
The application now resolves for the default branch of a repository
if the branch name is not given. It will also return an error if the
branch name given does not exist in the repository.

Resolves : #159

- Added requirements.txt file - Added venv/ folder to .gitignore - The application now resolves for the default branch of a repository if the branch name is not given. It will also return an error if the branch name given does not exist in the repository. Resolves : jupyterhub#159

welcome · 2021-05-28T21:55:12Z

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

yuvipanda

This is great! I left some comments around code style, and will try to find some time to test locally in the next few days.

For tests, can you add a few more tests that check for branch names being other than master ('main', 'random-something', etc)? Both for original cloning, and for subsequent pulls?

Thanks!

yuvipanda · 2021-05-30T09:40:17Z

nbgitpuller/pull.py

+        This checks to make sure the branch we are told to access
+        exists in the repo
+        """
+        p_heads = subprocess.run(


We should handle possible errors here. We can either pass check=True (or use subprocess.check_output), which will raise an exception if the process exits with a non zero code, or check the status code output directly.

This throws a subprocess.CalledProcessError; I am going to log the exception and then raise a ValueError that I imagine should be passed to the UI in some interprettable sentence by the user. (I was struggling with this peice; it is unclear to me if the handling of errors on the UI side is working. I have a couple examples where it did not seem to be which I thought should be a different issue/PR if indeed there is problem and I am not screwing something up!)

yuvipanda · 2021-05-30T09:40:30Z

nbgitpuller/pull.py

+            capture_output=True,
+            text=True,
+        )
+        p_tags = subprocess.run(


What does the p_ prefix stand for?

p = process = subprocess (!) -- I got rid of the "p_"

yuvipanda · 2021-05-30T09:40:53Z

nbgitpuller/pull.py

+        This will resolve the default branch of the repo in
+        the case where the branch given does not exist
+        """
+        p = subprocess.run(


Same comment about error handling here, and about the p_ prefix

yuvipanda · 2021-05-30T10:03:29Z

requirements.txt

@@ -0,0 +1,3 @@
+traitlets


So we specify notebook in install_requires in setup.py, which pulls in traitlets as a transitive dependency, so it shouldn't be necessary here. Since pytest isn't required when running the code, it is often listed in a dev-requirements.txt instead, and we add installing that to docs/contributing.md. I think the -e . is often not set up in dev-requirements.txt either, and it's explicitly just specified in docs/contributing.md - not sure exactly why, but that is the general consensus I've seen.

Some packages behave differently when they're installed in editable mode, so it's better not to force an installation method so a developer has the freedom to test the package installed in production mode (particularly important for CI), or to install in editable mode for development and accept there may be differences.

I am with you here. good deal

yuvipanda · 2021-05-30T10:04:33Z

nbgitpuller/pull.py

@@ -70,11 +70,60 @@ def __init__(self, git_url, branch_name, repo_dir, **kwargs):
        assert git_url and branch_name

        self.git_url = git_url
-        self.branch_name = branch_name
+
+        if branch_name == "None":


Suggested change

if branch_name == "None":

if branch_name is None:

Since None is a special object in Python, rather than a string.

It isn't the NoneType type that gets pased back through the handlers. I drove myself in a couple circles trying to figure out why "is None" wasn't working and realized the handlers were passing Strings. Maybe I am missing something here.

I dug around some more. SyncHandler services the api endpoint, which is called by the JS in static/index.js. I see the following request being made by JS: http://localhost:8888/git-pull/api?repo=https://github.com/yuvipanda/requirements&branch=None&targetpath=requirements. See that branch is explicitly set to None - so it's something in the JS.

The JS actually gets this from Python, in

nbgitpuller/nbgitpuller/static/index.js

Line 132 in 814682b

utils.get_body_data('branch'),

.

And here we put it in the HTML, in Python:

nbgitpuller/nbgitpuller/templates/status.html

Line 8 in 814682b

data-branch="{{ branch | urlencode }}"

.

You can see that depth is treated as optional, and only put in there if it is not None. However, branch is not considered as optional, and put in there all the time! So we need to add a similar conditional to branch, and only add it there if the branch argument is present.

nbgitpuller/nbgitpuller/static/index.js

Line 45 in 814682b

var syncUrlParams = {

will also need to be amended to make branch operate in a similar fashion to depth, and

nbgitpuller/nbgitpuller/handlers.py

Line 55 in 814682b

branch = self.get_argument('branch')

as well. There might be a couple other places that assume branch is always present, we'll have to track those down too.

yuvipanda · 2021-05-30T13:15:04Z

I just tested these two URLs, one with 'master' and one with 'main':

Both initial clone and further pulls seem to work! \o/

yuvipanda · 2021-05-30T13:42:49Z

the test failure is a small issue with python code style I think

yuvipanda · 2021-06-02T14:32:17Z

@sean-morris In #181, I've added a dev-requirements.txt, documented it, and documented how to run tests / linting / docs locally. I hope that is useful. We should remove the dev-requirements.txt from here to simplify merging though.

yuvipanda · 2021-06-02T15:30:14Z

@sean-morris ah, so that PR I opened also has the same failing test - https://github.com/jupyterhub/nbgitpuller/runs/2728756643?check_suite_focus=true. Not sure what's happening there.

sean-morris · 2021-06-02T20:44:31Z

@sean-morris ah, so that PR I opened also has the same failing test - https://github.com/jupyterhub/nbgitpuller/runs/2728756643?check_suite_focus=true. Not sure what's happening there.

@yuvipanda
My theory is that the url, with the url query string containing branch=master, is returning a 404 error -- but I could be wrong here. I have not seen anything online that indicates others struggling with a similar issue.

- removed requirements.txt - misguided moment! - added exception handling to subprocess.run - added tests for exception handling - added tests for checking that branch does exist

yuvipanda

I dug into why we're getting a string value of None for branch, and left some comments on what I found + a possible fix. I think we should sort that out before merging.

yuvipanda · 2021-06-07T11:14:48Z

nbgitpuller/pull.py

@@ -70,11 +70,60 @@ def __init__(self, git_url, branch_name, repo_dir, **kwargs):
        assert git_url and branch_name

        self.git_url = git_url
-        self.branch_name = branch_name
+
+        if branch_name == "None":


I dug around some more. SyncHandler services the api endpoint, which is called by the JS in static/index.js. I see the following request being made by JS: http://localhost:8888/git-pull/api?repo=https://github.com/yuvipanda/requirements&branch=None&targetpath=requirements. See that branch is explicitly set to None - so it's something in the JS.

yuvipanda · 2021-06-07T11:16:32Z

nbgitpuller/pull.py

@@ -70,11 +70,60 @@ def __init__(self, git_url, branch_name, repo_dir, **kwargs):
        assert git_url and branch_name

        self.git_url = git_url
-        self.branch_name = branch_name
+
+        if branch_name == "None":


The JS actually gets this from Python, in

nbgitpuller/nbgitpuller/static/index.js

Line 132 in 814682b

utils.get_body_data('branch'),

.

yuvipanda · 2021-06-07T11:21:04Z

nbgitpuller/pull.py

@@ -70,11 +70,60 @@ def __init__(self, git_url, branch_name, repo_dir, **kwargs):
        assert git_url and branch_name

        self.git_url = git_url
-        self.branch_name = branch_name
+
+        if branch_name == "None":


And here we put it in the HTML, in Python:

nbgitpuller/nbgitpuller/templates/status.html

Line 8 in 814682b

data-branch="{{ branch | urlencode }}"

.

You can see that depth is treated as optional, and only put in there if it is not None. However, branch is not considered as optional, and put in there all the time! So we need to add a similar conditional to branch, and only add it there if the branch argument is present.

nbgitpuller/nbgitpuller/static/index.js

Line 45 in 814682b

var syncUrlParams = {

will also need to be amended to make branch operate in a similar fashion to depth, and

nbgitpuller/nbgitpuller/handlers.py

Line 55 in 814682b

branch = self.get_argument('branch')

as well. There might be a couple other places that assume branch is always present, we'll have to track those down too.

The following changes ensure branch is only passed when it has been defined: - branch becomes keyword parameter to GitPuller - branch only addd to query string when not undefined - branch only encoded into url when exists

yuvipanda · 2021-06-08T22:06:04Z

\o/ Thank you very much, @sean-morris!

Brings in jupyterhub/nbgitpuller#179

yuvipanda requested changes May 30, 2021

View reviewed changes

sean-morris force-pushed the master branch from 5673438 to 69d8da8 Compare June 3, 2021 18:22

Updates to handling default or non-existing branch

8423af4

- removed requirements.txt - misguided moment! - added exception handling to subprocess.run - added tests for exception handling - added tests for checking that branch does exist

sean-morris force-pushed the master branch from 69d8da8 to 8423af4 Compare June 3, 2021 18:27

yuvipanda requested changes Jun 7, 2021

View reviewed changes

yuvipanda mentioned this pull request Jun 7, 2021

Default branch set to master in nbgitpuller 2i2c-org/infrastructure#405

Closed

2 tasks

Cleaned up passing of branch on query string

12f25dd

The following changes ensure branch is only passed when it has been defined: - branch becomes keyword parameter to GitPuller - branch only addd to query string when not undefined - branch only encoded into url when exists

yuvipanda merged commit 8d65dfc into jupyterhub:master Jun 8, 2021

consideRatio added bug enhancement and removed bug labels Jun 9, 2021

consideRatio changed the title ~~Handle default or non-existing branch name~~ Automatically detect default branch name Jun 9, 2021

yuvipanda added a commit to yuvipanda/datahub that referenced this pull request Jun 9, 2021

Bump nbgitpuller

153c540

Brings in jupyterhub/nbgitpuller#179

yuvipanda mentioned this pull request Jun 9, 2021

Bump nbgitpuller berkeley-dsep-infra/datahub#2423

Merged

yuvipanda added a commit to yuvipanda/pilot-hubs that referenced this pull request Jun 9, 2021

Bump nbgitpuller

e3bbe25

Brings in jupyterhub/nbgitpuller#179

yuvipanda mentioned this pull request Jun 9, 2021

Bump nbgitpuller 2i2c-org/infrastructure#459

Merged

yuvipanda added a commit to berkeley-dsep-infra/stat159-image that referenced this pull request Mar 29, 2022

Bump nbgitpuller

ba9cbb0

Brings in jupyterhub/nbgitpuller#179

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically detect default branch name #179

Automatically detect default branch name #179

sean-morris commented May 28, 2021

welcome bot commented May 28, 2021

yuvipanda left a comment

yuvipanda May 30, 2021

sean-morris Jun 1, 2021

yuvipanda May 30, 2021

sean-morris Jun 1, 2021

yuvipanda May 30, 2021

sean-morris Jun 1, 2021

yuvipanda May 30, 2021

manics May 30, 2021

sean-morris Jun 1, 2021

yuvipanda May 30, 2021

sean-morris Jun 1, 2021 •

edited

yuvipanda Jun 7, 2021

yuvipanda Jun 7, 2021

yuvipanda Jun 7, 2021

yuvipanda commented May 30, 2021

yuvipanda commented May 30, 2021

yuvipanda commented Jun 2, 2021

yuvipanda commented Jun 2, 2021

sean-morris commented Jun 2, 2021 •

edited

yuvipanda left a comment

yuvipanda Jun 7, 2021

yuvipanda Jun 7, 2021

yuvipanda Jun 7, 2021

yuvipanda commented Jun 8, 2021

Automatically detect default branch name #179

Automatically detect default branch name #179

Conversation

sean-morris commented May 28, 2021

welcome bot commented May 28, 2021

yuvipanda 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

sean-morris Jun 1, 2021 • edited

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

yuvipanda commented May 30, 2021

yuvipanda commented May 30, 2021

yuvipanda commented Jun 2, 2021

yuvipanda commented Jun 2, 2021

sean-morris commented Jun 2, 2021 • edited

yuvipanda 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

yuvipanda commented Jun 8, 2021

sean-morris Jun 1, 2021 •

edited

sean-morris commented Jun 2, 2021 •

edited