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

[GitLab] Use log.Error instead of log.Fatal for json parsing errors #37

Merged
merged 1 commit into from
Jan 23, 2019

Conversation

ManiacTwister
Copy link
Contributor

In the gitlabmodule log.Fatal is called when the module receives invalid/unexpected json data, which leads to program termination with exit status 1 - therefore it is possible to remotely "crash" CptHook. I think Log.Panic is more appropriate here?

@fleaz
Copy link
Owner

fleaz commented Jan 14, 2019

Hi,
thanks for reporting this and and an even bigger thanks to already providing a PR :)
The current behaviour is in fact not great.

According to the Logrus documentation Log.Panic calls the panic() function to gracefully shutdown stuff after logging the error. Wouldn't it be more sensible to just use Log.Error() in the cases where we receive malformed json?

@ManiacTwister ManiacTwister changed the title [GitLab] Use log.Panic instead of log.Fatal for json parsing errors [GitLab] Use log.Error instead of log.Fatal for json parsing errors Jan 14, 2019
@ManiacTwister
Copy link
Contributor Author

Yeah makes sense :D
Changed it to Log.Error

@fleaz fleaz changed the base branch from master to development January 14, 2019 19:14
@fleaz
Copy link
Owner

fleaz commented Jan 22, 2019

Hi @ManiacTwister I fixed some issues with my branches and now this PR should be clean again if you rebase against development. Then I will be happy to merge it :)

@ManiacTwister
Copy link
Contributor Author

@fleaz rebased, looks good now

@fleaz fleaz merged commit 194d161 into fleaz:development Jan 23, 2019
@fleaz
Copy link
Owner

fleaz commented Jan 23, 2019

Awesome. Thanks :)

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.

2 participants