The code LGTM but may be @firstyear would like to take a look at DatabaseConfig(DSLdapObject) implementation. Maybe I missed something in its idea.

One small nitpick though... You named the Commit after the Issue but there is no UI work in the PR.
It is confusing, IMHO...

The code LGTM but may be @firstyear would like to take a look at DatabaseConfig(DSLdapObject) implementation. Maybe I missed something in its idea.

Okay so the issue is that prior to 1.4.2 we had one config entry for the global database:

cn=config,cn=ldbm database,cn=plugins,cn=config

But in 1.4.2 we split this single entry into two entries:

• cn=config,cn=ldbm database,cn=plugins,cn=config
• cn=bdb,cn=config,cn=ldbm database,cn=plugins,cn=config

and eventually we'll have:

• cn=lmdb,,cn=config,cn=ldbm database,cn=plugins,cn=config

This fix basically merges these two entries into a single manageable object. This also fixes the UI, which is currently broken. Currently the UI only allows you to manage the cn=bdb entry, and not the global entry. So this is now fixed in this patch by just merging the config.

Then, to the user this is still one config object you need to manage, and the user does not need to worry about if it's BDB or LMDB, or whatever the underlying db library is. It's just one "entry" that handles the global database configuration for the server. Otherwise we need to add new CLI options for each database type, in an already crowded CLI option list. So I decided it would be easier for users, and cleaner for developers to just merge the pairing. Hope that helps clear up what this patch is trying to do.

If we already have get() method, can we also add display() method to DatabaseConfig object? I think it'll be better to have the tool in the API and not implementing the for loop here...

Looks l like I can clean up the lib389 part a bit and make it cleaner, so a new commit is coming...

If we already have get() method, can we also add display() method to DatabaseConfig object? I think it'll be better to have the tool in the API and not implementing the for loop here...

Yeah sounds good, I'll get that added!

changes made please review....

Probably worth checking that the server "enforces" that cn=bdb is the only valid cn on a bdb config.

It could be worth a safety check to ensure that set(_global_attrs) and set(_db_attrs) has no overlap, else this function may be ambiguous about what we are changing.

I think generally I'm happy with this. I think the "combined" set and get seems confusing, but also seems like "the right thing" here because a bdb only exists with the backend config so I think it's the right answer here. It could cause some confusion internally though with get/set vs get_attr etc.

I was expecting this list follows bdb_config_param but it does not. For example nsslapd-dbncache, nsslapd-db-page-size... are not in that list. Is it expected ?
If it should follow bdb_config_param, could you order the list like in bdb_config_param struct.

Same as below, should it match ldbm_config ?

I was expecting this list follows bdb_config_param but it does not. For example nsslapd-dbncache, nsslapd-db-page-size... are not in that list. Is it expected ?
If it should follow bdb_config_param, could you order the list like in bdb_config_param struct.

Well right now the CLI does not have "set" options for every single BDB setting. Instead we limited the settings to the most commonly used ones. So this list in sync with what the CLI can do. I could add more attributes here but it wouldn't really do anything since there is no way to set attributes like "nsslapd-maxpassbeforemerge" via dsconf. If we want to add every single attribute then we should do so in a different ticket.

Thanks @mreynolds for the explanation. You are correct it is not useful to support all corner cases db setting in a CLI. You have my ACK

Probably worth checking that the server "enforces" that cn=bdb is the only valid cn on a bdb config.

It is

It could be worth a safety check to ensure that set(_global_attrs) and set(_db_attrs) has no overlap, else this function may be ambiguous about what we are changing.

Sorry I'm not sure what you asking me to do. Do you want me to add a check that the attribute being updated it not in two lists? That doesn't seem necessary as the lists are maintained in the object itself. Please clarify, thanks!

It could be worth a safety check to ensure that set(_global_attrs) and set(_db_attrs) has no overlap, else this function may be ambiguous about what we are changing.

Sorry I'm not sure what you asking me to do. Do you want me to add a check that the attribute being updated it not in two lists? That doesn't seem necessary as the lists are maintained in the object itself. Please clarify, thanks!

Ahhh, yes. So maybe something like:

def __init__(self, ...): 
    assert_c(len(set(self._global_attrs).intersection(set(self._db_attrs)) == 0)

Probably just once in the init to make sure as programmers we got this right, so that def set and def get is sure that it's attributes are only from "one source".

It could be worth a safety check to ensure that set(_global_attrs) and set(_db_attrs) has no overlap, else this function may be ambiguous about what we are changing.
Sorry I'm not sure what you asking me to do. Do you want me to add a check that the attribute being updated it not in two lists? That doesn't seem necessary as the lists are maintained in the object itself. Please clarify, thanks!

Ahhh, yes. So maybe something like:
def init(self, ...):
assert_c(len(set(self._global_attrs).intersection(set(self._db_attrs)) == 0)

Done, merging...

Thanks mate, I'm glad you saw the invisible Ack that I forgot to put in (OPPS!!!). Ack for certain :)

Thanks mate, I'm glad you saw the invisible Ack that I forgot to put in (OPPS!!!). Ack for certain :)

Since you did not have any other concerns I assumed it was a ACK :-p Plus I wanted to get upstream builds done, but looks like master branch is currently broken :-( BUILD_NUM is not getting set on F33, investigating...

BUILD_NUM is not getting set on F33, investigating...

Can't locate FileHandle.pm in @INC (you may need to install the FileHandle module) (@INC contains: /usr/local/lib64/perl5/5.30 /usr/local/share/perl5/5.30 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5 /usr/share/perl5) at ./buildnum.pl line 22.

Looks like a missing perl dependency.

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/3997

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.