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

improved error and info logs when loading configurations #300

Merged
merged 1 commit into from
May 17, 2023
Merged

improved error and info logs when loading configurations #300

merged 1 commit into from
May 17, 2023

Conversation

engelmi
Copy link
Member

@engelmi engelmi commented May 12, 2023

This PR addresses #299

src/libhirte/common/cfg.c Outdated Show resolved Hide resolved
@mwperina
Copy link
Member

Hi Douglas,
configuration files handling has been implemented according to following issues:

#147
#148
#172

The PR #253 changed this a bit, but still following rules applies:

  1. If /etc/hirte/{hirte|agent}/conf doesn't exist, we should not produce any error
  2. If /etc/hirte/{hirte|agent}/conf exist, but it contains an error inside the file, we should fail the startup (clearly user error)
  3. If /etc/hirte/{hirte|agent}.conf.d doesn't exist or it's empty (we care only about files with .conf suffix), we should not produce any error
  4. If /etc/hirte/{hirte|agent}.conf.d exists and it contains a file with .conf suffix, which contains an error inside it, we should fail the startup (clearly user error)

The only planned changes around configuration are described at #301

Copy link
Member

@mwperina mwperina left a comment

Choose a reason for hiding this comment

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

Except the error message content change it looks good to me

src/libhirte/common/cfg.c Outdated Show resolved Hide resolved
@dougsland
Copy link
Contributor

Hi Douglas, configuration files handling has been implemented according to following issues:

#147 #148 #172

The PR #253 changed this a bit, but still following rules applies:

  1. If /etc/hirte/{hirte|agent}/conf doesn't exist, we should not produce any error
  2. If /etc/hirte/{hirte|agent}/conf exist, but it contains an error inside the file, we should fail the startup (clearly user error)
  3. If /etc/hirte/{hirte|agent}.conf.d doesn't exist or it's empty (we care only about files with .conf suffix), we should not produce any error
  4. If /etc/hirte/{hirte|agent}.conf.d exists and it contains a file with .conf suffix, which contains an error inside it, we should fail the startup (clearly user error)

The only planned changes around configuration are described at #301

Hello @mwperina Thanks for sharing the info. I am not saying the RFEs created to manage the config files are incorrect. I am just seeing some improvements are possible here. However, let's move forward with @engelmi work. I will work this week to prepare a proof of concept and send a PR.

Thanks guys!

src/libhirte/common/cfg.c Outdated Show resolved Hide resolved
@dougsland
Copy link
Contributor

/retest
/lgtm

Signed-off-by: Michael Engel <mengel@redhat.com>
Copy link
Member

@mwperina mwperina left a comment

Choose a reason for hiding this comment

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

+1

@engelmi engelmi merged commit 0f5ae95 into eclipse-bluechi:main May 17, 2023
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.

4 participants