jupyterhub / nbgitpuller

Jupyter server extension to sync a git repository one-way to a local path
https://nbgitpuller.readthedocs.io
BSD 3-Clause "New" or "Revised" License
212 stars 86 forks source link

Provide better error messages in the terminal #267

Open yuvipanda opened 2 years ago

yuvipanda commented 2 years ago

Right now, the terminal window in nbgitpuller shows the sequence of git commands being executed and their output. However, when there is a failure, users are just left with the raw output of the git command - which can be intimidating and confusing. It would be useful to provide more useful error messages here for at least common occurances.

balajialg commented 2 years ago

@yuvipanda John De Nero was hoping that the error messages students get when the git conflict issue happens will get improved to include meaningful error messages which are actionable for students (in the upcoming times). Particularly, he hinted at the use case where the instructor makes changes to the Github repository after it gets shared with students. Is this something within the scope of this Github issue? If yes. is there a nbgitpuller roadmap where I can see whether this particular issue is scoped in the near term?

balajialg commented 2 years ago

@yuvipanda Multiple GSIs complained about this error message issue yesterday! I guess they spent more time debugging this issue during the Spring semester. Is it possible to improve the error message so that GSIs don't spend a lot of time debugging this issue? Happy to frame better error messages if required.

yuvipanda commented 2 years ago

@balajialg part of the problem is that the design guideline is to never have conflicts (https://jupyterhub.github.io/nbgitpuller/topic/automatic-merging.html), and when conflicts do happen it's already an error. And unfortunately the git error message appearing already means that all our handling of the cases has failed, so it's a bit of 'unknown' territory

So I'd say the way to improve this is to:

  1. Identify exactly when a merge conflict error message occurs. What is the sequence of events that led to it?
  2. Fix nbgitpuller so the merge conflict doesn't happen.

So if you have some way to reproduce the error the users were facing, that would be helpful!

yuvipanda commented 2 years ago

Could be helped by https://github.com/jupyterhub/nbgitpuller/pull/269

yuvipanda commented 2 years ago

269 will help with one set of merge conflict errors, but I suspect maybe the GSIs you spoke to were running into something else, @balajialg. But each error is different, and we'll have to:

  1. Detect why there's an unresolved conflict
  2. Find a way to resolve it automatically
  3. If (2) is not possible, provide a more useful error message.

In #269, @jdmansour detected why there was an unresolved conflict (#265) and then found a way to auto resolve it (#269). We'll have to do that for other errors too.

@balajialg so a useful thing would be to capture the output on the terminal when it fails and report it here. That might help us perform these steps.

ryanlovett commented 2 years ago

@yuvipanda Could there be a configurable error message instructing people where to report errors?

balajialg commented 2 years ago

@yuvipanda This is great! I will follow up with GSIs to nudge them to report error snapshots in this Github issue directly. Based on my understanding, Case 1 seems to be the scenario most described by GSIs. Considering that it is handled effectively (based on your documentation), I will probe them further to reproduce the steps so that we can identify the exact cause of the issue effectively.

jdmansour commented 2 years ago

I'd like to come back to this issue. We sometimes get sync errors, and it is hard to find out what is going on even for advanced git users, since the git output is not always shown. For example:

grafik

(The reason is that we yield the result of git line by line, so that it can be streamed to the terminal. But some functions like branch_exists return a value and can't yield the output. I'm not sure what the most elegant way would be to fix this. In the error case, we could wrap the error in a GitError which would contain the git output.)

I think there are three levels here, with increasing difficulty:

yuvipanda commented 2 years ago

@jdmansour the three levels you lay out seem great! Making sure all CalledProcessErrors provide readable output could also be a first start