Skip to content
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

787 improve ssh access error handling #873

Merged
merged 6 commits into from
May 8, 2014

Conversation

0robustus1
Copy link
Contributor

Shall improve error handling of git/ssh.
It fixes #787.

The following elements are added:

  • custom error messages to the users instead of stacktraced errors
  • Error Logging into Log-File as soon as possible (after environment/logger is loaded).

end

def generate_message
<<-ERROR.squish
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't use squish here because not all callers have the rails environment loaded

This includes some error classes, a default
no-redirections-limit constant (which uses the value 1)
and a specific TooManyRedirects error class, which also
contains the previous response which we received
before it all went to hell.
Also produce more useful error message inside
of the log (server-side).
When ssh-access querying for permissions fails
for HTTP related reasons the git-push user should be
informed of the situation instead of being attacked
by a ruby stacktrace.
end
puts <<-ERROR
We encountered a system error while processing your
push attempt. The push was not attempted and your data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"... processing your push attempt. The push was not attempted"
I'm no expert on the english language, but that sounds a little weird to me. Maybe replace the last "not attempted" by "not completed", "not finished" or "aborted".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirely sure why you're objecting (attempted). Technically it is true: we did not attempt to actually push data from the client to server (or vice versa).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer something like "The push command was not processed"

@eugenk
Copy link
Member

eugenk commented May 2, 2014

Not directly concerining your changes (while still concerning messages to the user), but in https://github.com/ontohub/ontohub/blob/787-improve_ssh_access_error_handling/git/lib/git_shell.rb#L38 you don't use $stderr.puts while using it a few lines above that. Is there a reason for doing so?

@0robustus1
Copy link
Contributor Author

Mh, i guess the only reason is that i didn't notice (the $stderr.puts line are from corny).

To be honest, i'm not entirely sure if it makes a difference, as i do not know if it would actually print to standard error on the client (as there is a network connection and git-handling between printing the message and the user receiving it). This could/should probably investigated.

However we shouldn't use $stderr. Better to use STDERR instead (they are object-identity equivalent, but using global variables always give me some sort of a bad feeling).

@0robustus1
Copy link
Contributor Author

I made the adjustments in the error text. I've also replaced "push" with "git-interaction" and "git command", as at least one of error messages could also be triggered by a git fetch or git clone.

I have also changed the puts calls from implicit STDOUT to explicit STDERR, and replaced $stderr with STDERR.

@eugenk
Copy link
Member

eugenk commented May 8, 2014

👍

0robustus1 added a commit that referenced this pull request May 8, 2014
…dling

787 improve ssh access error handling
@0robustus1 0robustus1 merged commit f89b4aa into staging May 8, 2014
@0robustus1 0robustus1 deleted the 787-improve_ssh_access_error_handling branch May 8, 2014 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve exception handling for SSH access
3 participants