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

ServiceConfig: log errors when loading config #34691

Closed
wants to merge 1 commit into from

Conversation

vrothberg
Copy link

Log errors when the config is loaded to notify the user of any errors,
for instance, when a specified mirror has an invalid URL.

Signed-off-by: Valentin Rothberg vrothberg@suse.com

- What I did / How I did it

Catch and log errors when loading the config in newServiceConfig().

- How to verify it

Start the daemon for instance with --registry-mirrors "in@valid:url". The daemon starts successfully, but now we can see the following log: level=warning msg="error loading mirrors: invalid mirror: \"in@valid:url\" is not a valid URI"

- Description for the changelog

Log errors when the config is loaded.

Log errors when the config is loaded to notify the user of any errors,
for instance, when a specified mirror has an invalid URL.

Signed-off-by: Valentin Rothberg <vrothberg@suse.com>
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

LGTM but why aren't we returning err?

@boaz0
Copy link
Member

boaz0 commented Aug 31, 2017

wait... isn't it a duplicate of #34495 ?

@vrothberg
Copy link
Author

vrothberg commented Aug 31, 2017

LGTM but why aren't we returning err?

@AkihiroSuda We could also return the error here and make checks in the callers, but as we only use it for logging, I thought that's fine.

wait... isn't it a duplicate of #34495 ?

@ripcurld0 Thanks for making the link! I had a look this PR now, and would object that it's likely to make things complicated for #34319 as --registry-mirror may also serve private registries in the future. I will look closer at #34495 and leave a comment.

@boaz0
Copy link
Member

boaz0 commented Aug 31, 2017

@vrothberg

I got the impression that if there is an error we would like to stop daemon execution.
So it depends what is the right approach here. To print the error as a warning or exit with an error message.

@vrothberg
Copy link
Author

@ripcurld0

You're right, it also aligns with what #34476 suggests to do. I think that #34495 is the right place to continue. I will close this PR to avoid confusion and redundant reviews. Thanks a lot for checking!

@vrothberg vrothberg closed this Aug 31, 2017
@vrothberg vrothberg deleted the log-config-load branch August 31, 2017 11:40
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.

4 participants