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 a hint to resolve a misconfigured helper_binaries_dir #813

Merged
merged 2 commits into from Oct 30, 2021

Conversation

jre21
Copy link
Contributor

@jre21 jre21 commented Oct 23, 2021

This change is based on discussion in containers/podman#11960. I don't especially like the way the multi-sentence style clashes with other log messages, especially since this is typically encapsulated within a higher-level error, but couldn't think of a better way to convey the relevant information. I'm open to suggestions on a better approach.

Signed-off-by: Jacob Emmert-Aronson <jacob@roadnottaken2718.com>
@jre21 jre21 changed the title Add a hint for misconfigured helper_binaries_dir Add a hint to resolve a misconfigured helper_binaries_dir Oct 23, 2021
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Show resolved Hide resolved
pkg/config/config.go Show resolved Hide resolved
Co-authored-by: Valentin Rothberg <rothberg@redhat.com>
@jre21
Copy link
Contributor Author

jre21 commented Oct 27, 2021

Are there any other cases where podman uses multi-sentence log messages, or otherwise tries to include more context than a terse description of the error? I'm applying @vrothberg's suggested changes because I think using sentence case in these circumstances makes the most sense, but this is also the sort of stylistic decision that works best when made deliberately by the project maintainers.

@vrothberg
Copy link
Member

Are there any other cases where podman uses multi-sentence log messages, or otherwise tries to include more context than a terse description of the error?

Yes, multi-sentence log messages occur frequently, for instance https://github.com/containers/common/blob/main/libimage/layer_tree.go#L120. But it really depends on the case. In case of the layer tree, we'd suppress an error and hence need to log to inform the user that something fishy may have happened.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@rhatdan
Copy link
Member

rhatdan commented Oct 28, 2021

I still don't see the reason to capitalize the error, just because their is a follow on sentence.
The way Podman and Buildah will report this is.

"Error: could not ... To resolve this error, set the helper_binaries_dir key in the [engine] section of containers.conf to the directory containing your helper binaries."

But with the final patch it looks like:

"Error: Could not ... To resolve this error, set the helper_binaries_dir key in the [engine] section of containers.conf to the directory containing your helper binaries."

Which will be different then every other Error reported. I think the error in the first instance looks fine and follows the standard.

@marchmallow
Copy link

As a user, if I were to get the new error message vs. the old error message.. I would still be a long way from understanding how to fix the issue.. as if I did not read the discussion in containers/podman#11960 I would not had a clue where container.conf was and what I would had to add to it, also considering that that conf did not contain any helper_binaries_dir key before I edited it.. see my comments on the issue containers/podman#11960 (comment) and following..

@rhatdan
Copy link
Member

rhatdan commented Oct 28, 2021

@marchmallow I am not arguing against this fix, this is simply an argument about whether the "could not" should be "Could not". If we remove the third commit, I would merge this now.

@jre21
Copy link
Contributor Author

jre21 commented Oct 29, 2021

I reverted the capitalization changes. The note about the error messages being lowercased because the underlying machinery adds its own prefixes makes a lot of sense to me.

I think marchmallow's comment points to a deeper issue: podman's configuration isn't well-documented. In looking through the docs, I found this section describing how podman searches for configuration files, but power users typically want a reference for the format and structure of this file and all the settings that are available to adjust, and that reference doesn't seem to exist yet.

Writing that reference and defining a (ideally at least partially-automated) process to ensure it remains up to date when code changes add new configuration variables would be a huge service to the community, but is also well out of scope for this particular pull request.

@rhatdan
Copy link
Member

rhatdan commented Oct 30, 2021

/approve
/lgtm

@rhatdan
Copy link
Member

rhatdan commented Oct 30, 2021

Thanks @jre21

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 30, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jre21, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 2dd5d30 into containers:main Oct 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants