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

Add message to errors which fail hook run #3

Closed
wants to merge 1 commit into from

Conversation

ashcrow
Copy link
Contributor

@ashcrow ashcrow commented Jun 7, 2017

Add "Failing hook run" to error messages which cause the hook to fail.
Error messages which are skipped or shown for information only are left
alone.

Add "Failing hook run" to error messages which cause the hook to fail.
Error messages which are skipped or shown for information only are left
alone.
@rhvgoyal
Copy link
Collaborator

rhvgoyal commented Jun 7, 2017

We already prefix string "error" in error messages using "pr_perror()" macro. And that makes it pretty clear that hook failed and it encountered an error. Otherwise it will be either info or warning.

So I am not convinced that we should add "Failing running hook" in each error message.

@ashcrow
Copy link
Contributor Author

ashcrow commented Jun 7, 2017

@rhvgoyal I understand. It's your call, but keep in mind it's also appended to errors which don't cause the hook to fail.

Here is an example:

if (ret < 0) {
    pr_perror("Error while trying to map mount [%s] from host to conatiner. Skipping.\n", mounts_on_host[i]);
    continue;
}

@rhvgoyal
Copy link
Collaborator

rhvgoyal commented Jun 7, 2017

@ashcrow May be we should change these skipping messages to pr_pwarning() instead of pr_perror().

@ashcrow
Copy link
Contributor Author

ashcrow commented Jun 7, 2017

@rhvgoyal sure, that works. I'll update this PR and move to that.

@ashcrow
Copy link
Contributor Author

ashcrow commented Jun 7, 2017

Closed in favor of #4

@ashcrow ashcrow closed this Jun 7, 2017
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.

None yet

2 participants