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 full path for error messages on containers.conf #164

Merged
merged 1 commit into from
May 27, 2020

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented May 26, 2020

Also add new function to allow container engines to tell users
where to edit containers.conf file.

Signed-off-by: Daniel J Walsh [email protected]

@rhatdan
Copy link
Member Author

rhatdan commented May 26, 2020

@FlorianLudwig @mheon PTAL

@mheon
Copy link
Member

mheon commented May 26, 2020

LGTM

Copy link

@FlorianLudwig FlorianLudwig left a comment

Choose a reason for hiding this comment

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

I have limited golang and code base knowledge so I feel bad posting a "request for change" but here are my requested two cents, hope they are helpful :)

@@ -226,7 +226,7 @@ func newLibpodConfig(c *Config) error {

// hard code EventsLogger to "file" to match older podman versions.
if config.EventsLogger != "file" {
logrus.Debugf("Ignoring lipod.conf EventsLogger setting %q. Use containers.conf if you want to change this setting and remove libpod.conf files.", config.EventsLogger)
logrus.Debugf("Ignoring lipod.conf EventsLogger setting %q. Use %q if you want to change this setting and remove libpod.conf files.", Path(), config.EventsLogger)

Choose a reason for hiding this comment

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

lipod.conf should be libpod.conf

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

@@ -262,7 +262,7 @@ func systemLibpodConfigs() ([]string, error) {
}
// TODO: Raise to Warnf, when Podman is updated to
// remove libpod.conf by default
logrus.Debugf("Found deprecated file %s, please remove. Use %s to override defaults.\n", path, containersConfPath)
logrus.Debugf("Found deprecated file %s, please remove. Use %s to override defaults.\n", Path(), containersConfPath)

Choose a reason for hiding this comment

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

It seems the function Path is only used for logging but not the code path actually loading the configs, which might lead to a different path being logged and loaded.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well yes, but the code that does the actual search is also going to use /usr/share/containers/containers.conf for reading the distro configuration. But we don't want to indicate to users that they should use that configuration.

Choose a reason for hiding this comment

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

Sounds reasonable.

Also add new function to allow container engines to tell users
where to edit containers.conf file.

Signed-off-by: Daniel J Walsh <[email protected]>
@FlorianLudwig
Copy link

LGTM

@TomSweeneyRedHat
Copy link
Member

LGTM
@vrothberg or @mheon PTAL

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

@vrothberg vrothberg merged commit 0e6180b into containers:master May 27, 2020
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.

5 participants