This should stay as info to ALWAYS indicate clearly that no action is taken on dry run.

This doesn't look like a logging change ;)

Why are we changing stderr here? That seems incorrect ....

This change concerns me because the whole point of this logging was so that in the case of a failure we had the logs in ipa's install log still. So ... why are we removing this? It seems like "clean logs" matters less than "we have all the info needed to solve a problem".

This should stay as info to ALWAYS indicate clearly that no action is taken on dry run.
Why are we changing stderr here? That seems incorrect ....

Right. I was a bit harsh there... Setting it back.

This doesn't look like a logging change ;)

As I mentioned in the PR/Commit message, it is done because sepolicy has verbose output that appears on import (I've seen it only during ipa-server-install though...). Instead of developing a tricky workaround, direct semange call was used (it is still workaround but it is a straight forward one :) ).

This change concerns me because the whole point of this logging was so that in the case of a failure we had the logs in ipa's install log still. So ... why are we removing this? It seems like "clean logs" matters less than "we have all the info needed to solve a problem".

In case of a failure, we still have logging that is put to a separate file in IPA installation.
Also, I really see no benefit in systemctl enable/disable output on every (even non-verbose) run. The same is true for useless sepolicy import output (we have a separate check for selinux anyway).

I'm worried about you removing these - these are important to detect and avoid selinux calls on systems without selinux (ie suse :) ). If there is an issue with the log message, just change it to log.warn instead? But this needs to stay.

As does this.

After these two comments I think it's okay :) but those blocks shouldn't be removed.

I'm worried about you removing these - these are important to detect and avoid selinux calls on systems without selinux (ie suse :) ). If there is an issue with the log message, just change it to log.warn instead? But this needs to stay.

As I said previously, we have a check for selinux already...
It is done by import selinux; if selinux.is_selinux_enabled():

sepolicy is used only for acquiring labeled ports so we can relabel them.
And I can't set the logging to warn because it happens during import sepolicy and shown only during ipa-server-install (not sure why, some wierd logger setting?). I spent an hour trying to suppress the message and it already had become a weird workaround... So I decided to make a straight forward workaround. :)

Let's remove the initial newline from the text string, thanks

Let's remove the initial newline from the text string, thanks

Fixed

I also see this seem annoying behavior from systemd every time I create an instance

# dscreate from-file /data/dssetup.inf 
Starting installation...
Created symlink /etc/systemd/system/multi-user.target.wants/dirsrv@localhost.service → /usr/lib/systemd/system/dirsrv@.service.
Completed installation for localhost

I originally moved that message to DEVNULL as well but that change was rejected by William I think :-p I personally want to see these messages removed from STDOUT by whatever means possible. :-)

Sorry just a side comment that's partially related to this issue

Gosh that William kid, just causing trouble for everyone, sheesh ;) Who even let that person be on the team anyway ....

There is a difficult balance here - between too much detail when not needed, and not enough info when it is needed. My greatest concern is "when something breaks, can the admin resolve it with the information we provided?". This is why I always err to "too much detail" because in the good case, we wasted some terminal space. If we remove that detail, we now have people who can't solve their issues, have a negative experience and can't supply the needed details to us to fix their issues.

I think that the current setup of the log system in lib389 is all focused around stdout/stderr. What we need is a way to also have it write as debug level to a location and then we can have a lower level in the stdout/stderr levels.

So instead of directing these to devnull, we should direct them to a python buffer via subprocess, then we should write these buffers to the log.debug. From there we can have the log code setup the file back and stderr/stdout handlers.

Thoughts?

Also what about the selinux bits?

I also see this seem annoying behavior from systemd every time I create an instance
dscreate from-file /data/dssetup.inf

I moved it to debug level so it won't be bagging during the non-verbose installation.

So instead of directing these to devnull, we should direct them to a python buffer via subprocess, then we should write these buffers to the log.debug. From there we can have the log code setup the file back and stderr/stdout handlers.
Thoughts?

Fully agree.
I've moved it to the debug and we can add a FileHandler option for logging in a separate issue (and it will be more of a new feature, I guess).

Anyway, this issue is ready for review. :)

selinux comments? you haven't addressed these ....

Ahhh you refactored it and I missed it.
Ack from me then.

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3607

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.