New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Automatically detect default branch name #179
Conversation
- 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
Thanks for submitting your first pull request! You are awesome! 🤗 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
nbgitpuller/pull.py
Outdated
This checks to make sure the branch we are told to access | ||
exists in the repo | ||
""" | ||
p_heads = subprocess.run( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!)
nbgitpuller/pull.py
Outdated
capture_output=True, | ||
text=True, | ||
) | ||
p_tags = subprocess.run( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the p_
prefix stand for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p = process = subprocess (!) -- I got rid of the "p_"
nbgitpuller/pull.py
Outdated
This will resolve the default branch of the repo in | ||
the case where the branch given does not exist | ||
""" | ||
p = subprocess.run( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about error handling here, and about the p_
prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
requirements.txt
Outdated
@@ -0,0 +1,3 @@ | |||
traitlets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am with you here. good deal
nbgitpuller/pull.py
Outdated
@@ -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": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if branch_name == "None": | |
if branch_name is None: |
Since None
is a special object in Python, rather than a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JS actually gets this from Python, in
nbgitpuller/nbgitpuller/static/index.js
Line 132 in 814682b
utils.get_body_data('branch'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here we put it in the HTML, in Python:
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 = { |
branch
operate in a similar fashion to depth
, and nbgitpuller/nbgitpuller/handlers.py
Line 55 in 814682b
branch = self.get_argument('branch') |
branch
is always present, we'll have to track those down too.
I just tested these two URLs, one with 'master' and one with 'main':
Both initial clone and further pulls seem to work! \o/ |
the test failure is a small issue with python code style I think |
@sean-morris In #181, I've added a |
@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 |
- removed requirements.txt - misguided moment! - added exception handling to subprocess.run - added tests for exception handling - added tests for checking that branch does exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
nbgitpuller/pull.py
Outdated
@@ -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": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
nbgitpuller/pull.py
Outdated
@@ -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": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JS actually gets this from Python, in
nbgitpuller/nbgitpuller/static/index.js
Line 132 in 814682b
utils.get_body_data('branch'), |
nbgitpuller/pull.py
Outdated
@@ -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": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here we put it in the HTML, in Python:
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 = { |
branch
operate in a similar fashion to depth
, and nbgitpuller/nbgitpuller/handlers.py
Line 55 in 814682b
branch = self.get_argument('branch') |
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
\o/ Thank you very much, @sean-morris! |
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