Added --no-superuser flag #3204
Conversation
To allow import into already existing databases. Refs: osm-search#2719
|
Normally I implemented everything as described in the linked ticket. Thanks in advance |
There was a problem hiding this comment.
Thanks for working on this! I've yet to try this out, but the main use case for me would be running the preparation using the postgres superuser, then running the import as the unprivileged nominatim user. AFAICT this isn't possible yet without changing the config file inbetween, ideally the preparation step would accept different postgresql connection settings on the commandline.
|
@Xiretza Thanks for taking a look! I'll implement the changes as soon as possible. Splitting it up into different commands altogether will break compatibility I presume, so I'll first look at renaming the flags hoping that will clear the logic up (which indeed the logic for is kinda messy at the moment). |
Preparing database should work without osm-file
|
I find the options still confusing, especially as they now in competition with The first command can be prefixed with This should make much of the logic easier. |
Made sure legacy import is also still ok
|
@lonvia I Implemented the options using continue, but I kept the legacy option also. I wasn't sure so I opted to do it like this first. |
Fixed small oversight in mutually exclusiveness of arguments
No. The logic right now is what I had in mind. Makes it a reasonably simple change. Regarding github actions, I think you are almost there. Simply work from a copy of the install action of these lines: https://github.com/osm-search/Nominatim/blob/master/.github/workflows/ci-tests.yml#L222-L291 instead of modifying the existing one. Once you add the code lines you already have, that should already be it. Maybe at the call to |
Added nominatim_database_webuser (www-data). Set non-superuser password for importing
|
@lonvia So, the CI tests are failing. I fixed the create import user (-P option doesn't accept a string which was a wrong assumption on my end). However there are also some fails on the linting, but I simply do not know how to reduce the branching any further. Do I disable that test? Could also probably create a function which does some of the sanity checks (first three if statements). |
Yes, for this particular case you can simply add another exception in line 74 for the 'too-many-branches' check. That entire function will need some refactoring eventually but that is something for a different PR. |
Check whether prepare-database is true, if so exit early
Removed creation of user www-data as it already exists
|
I looked at the errors in the CI log, the failing postgres login is probably because it doesn't have a password set yet, which can be fixed easily. I've tested a non-superuser import on Nomad, and everything seems to work just fine. So I assume changing the signature could be ok? I'll take a more in-depth look tomorrow, today I was preoccupied with some other work. |
|
@lonvia My apologies for the really slow implementation of the change requests. |
|
Static type checking seems to pass, only thing left is getting the import user correct in the CI tests. |
|
Almost good to go. Could I kindly ask you to rebase your code on master and force push to the branch? If you prefer to squash your commits, you are welcome to do so as well. |
|
The failing check should be fixed on master. I've manually merged this now doing a rebase. Thank you for your work and for patiently addressing all comments. It would be good to have some documentation for this new feature in https://nominatim.org/release-docs/latest/admin/Advanced-Installations/. If you like doing a follow-up commit, it would mean adding a section in https://github.com/osm-search/Nominatim/blob/master/docs/admin/Advanced-Installations.md |
Adding the --no-superuser flag to allow imports into already existing databases.
Fixes: #2719