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
ipa-client-install: Update how comments are added by ipachangeconf #2093
ipa-client-install: Update how comments are added by ipachangeconf #2093
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I understand this correctly, if a value that IPA wants to modify already exists, it adds a comment that IPA modifieed the variable plus a commented out version of that variable?
So if TLS_CACERTDIR is pre-set then post IPA install one would have
# TLS_CACERTDIR modified by IPA
# TLS_CACERTDIR /some/other/path
TLS_CACERTDIR /user/provided/value
This seems like a reasonable fix for the fact that openldap can't parse in-line comments. The idea was to highlight those things that IPA changed hopefully so end-users don't modify them.
I added a separate test after yours to add a new option, VAR_NAME2, and it was not preceded by a comment "modified by IPA". Maybe my test was bad but I think an additional test like this is needed in any case.
@rcritten thanks for adding some explanation to this patch! Something I failed to do.
The comment "modified by IPA" proceeds only variables that IPA would override. However, I can test the scenario you've mentioned to assert that the result wouldn't include the comment. |
|
||
|
||
@pytest.fixture(scope='session') | ||
def config_file(tmpdir_factory): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tmpdir_factory
was introduced in pytest 2.8, but RHEL 7.5 has 2.7.0-2.el7. You need to use another way to create a temporary file, e.g. tmpdir
fixture with a function
scope fixture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice observation, would pr-ci nightly catch this eventually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. QE will eventually run into this and complain.
|
||
@pytest.fixture(scope='session') | ||
def config_file(tmpdir_factory): | ||
file = tmpdir_factory.mktemp('data').join('config_file.conf') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file
is the name of a Python 2 builtin. In other places, we use filename
to refer to a file name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad..
ipa_conf.changeConf(str(config_file), opts) | ||
|
||
assert config_file.readlines() == [ | ||
'#SOME_CONF modified by IPA\n', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#text
is hard to read. Could you change the code to emit # text
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
Due to how 'openldap-client' parses its configuration files this patch changes how comments are added, moving them to the line above instead of appending to the same line. IPA doesn't want to break existing configuration, if a value already exists it adds a comment to the modified setting and a note about that on the line above. New settings will be added without any note. Issue: https://pagure.io/freeipa/issue/5202 Signed-off-by: Armando Neto <abiagion@redhat.com>
@tiran I've changed to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
master:
|
Due to how 'openldap-client' parses its configuration files this patch changes how comments are added, moving them to the line above instead of appending to the same line.
Issue: https://pagure.io/freeipa/issue/5202