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

Issue 6468 - RFE - CLI - Add logging settings to dsconf #6469

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

mreynolds389
Copy link
Contributor

Description:

It would be nice to have a more friendly CLI interface for managing the server logging. Specifically the log levels. This PR adds a new subcommand "logging" that can handle each log type and all the settings for it.

relates: #6468

Copy link
Member

@droideck droideck left a comment

Choose a reason for hiding this comment

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

I think it should be possible to create helper functions to generate common parsers, reducing duplication. Most of the attributes repeat across different log types.

Something like?..
For all of them in a for loop (this part will really reduce the redundancy):

    _add_enable_disable_parsers(set_parsers, log_type)
    _add_location_parser(set_parsers, log_type)
    _add_compression_parsers(set_parsers, log_type)
    etc..

And the rest for special cases where we can't use for loop for all of them.

Another thing:
I think, we need to add a bit more validation where it's possible (time format and sizes?)

def test_log_settings(topo):
"""Test each log settings can be set successfully

:id: ffc912a6-c188-413d-9c35-7f4b3774d946
Copy link
Member

Choose a reason for hiding this comment

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

duplicate id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah meant to fix that but forgot. This was a side project of mine that I've slowly been working on for the last few months.

@mreynolds389
Copy link
Contributor Author

Another thing: I think, we need to add a bit more validation where it's possible (time format and sizes?)

We already do the validation in the core server (libglobs.c). I don't see the need to duplicate it in the CLI. The CLI will correctly report the error message from the server.

@droideck
Copy link
Member

droideck commented Jan 3, 2025

Another thing: I think, we need to add a bit more validation where it's possible (time format and sizes?)

We already do the validation in the core server (libglobs.c). I don't see the need to duplicate it in the CLI. The CLI will correctly report the error message from the server.

Yeah, I agree. I just worry that UI depends on desc field. And when we get Operations error, it is the only thing that is displayed in UI. (i.e. Error saving Audit Log settings - Operations error)

Also, CLI output looks a bit not clean:

# dsconf inst  logging audit set max-logsize 2147483647

Error: 103 - 3 - 1 - Operations error - [] - nsslapd-auditlog-logmaxdiskspace: maxdiskspace "100 (MB)" is less than max log size "2147483647 (MB)" - modify_ext_s(('cn=config', [(2, 'nsslapd-auditlog-maxlogsize', [b'2147483647'])]),{'serverctrls': None, 'clientctrls': None, 'escapehatch': 'i am sure'}) on instance inst

IMHO, it'll be nice to have a clean Error: nsslapd-auditlog-logmaxdiskspace: maxdiskspace "100 (MB)" is less than max log size "2147483647 (MB)" formatted error in CLI and UI.

But we can get the validation error from the exception and parse it correctly in UI and CLI. And we can deal with it in a different ticket, probablly.

So, yeah, discard this concern. :)

@mreynolds389
Copy link
Contributor Author

Also, CLI output looks a bit not clean:

# dsconf inst  logging audit set max-logsize 2147483647

Error: 103 - 3 - 1 - Operations error - [] - nsslapd-auditlog-logmaxdiskspace: maxdiskspace "100 (MB)" is less than max log size "2147483647 (MB)" - modify_ext_s(('cn=config', [(2, 'nsslapd-auditlog-maxlogsize', [b'2147483647'])]),{'serverctrls': None, 'clientctrls': None, 'escapehatch': 'i am sure'}) on instance inst

Ugh that is ugly

IMHO, it'll be nice to have a clean Error: nsslapd-auditlog-logmaxdiskspace: maxdiskspace "100 (MB)" is less than max log size "2147483647 (MB)" formatted error in CLI and UI.

But we can get the validation error from the exception and parse it correctly in UI and CLI. And we can deal with it in a different ticket, probablly.

Hmm, yeah we use generic error handling in the CLI. It was never clean. I think it would be nice to improve it - perhaps that will be my next side project :-)

Description:

It would be nice to have a more friendly CLI interface for managing the server
logging. Specifically the log levels. This PR adds a new subcommand "logging"
that can handle each log type and allthe settings for it.

relates: 389ds#6468

Reviewed by: spichugi(Thanks!)
@mreynolds389
Copy link
Contributor Author

@droideck Requested changes made. I just lumped all the settings into a single loop instead of having separate functions for common parsers

Copy link
Member

@droideck droideck left a comment

Choose a reason for hiding this comment

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

Looks great! Ack

@mreynolds389 mreynolds389 merged commit a7f7cd9 into 389ds:main Jan 6, 2025
199 checks passed
@mreynolds389 mreynolds389 deleted the dsconf_log_levels branch January 6, 2025 19:33
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.

2 participants