-
-
Notifications
You must be signed in to change notification settings - Fork 362
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
Initial database_setup
RPC functions
#3665
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.
Looks good to me!
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.
Looks great overall @mathemancer, I've pointed out a minor bug please see the specific comment below.
mathesar/utils/permissions.py
Outdated
name=role_name, | ||
server=server, | ||
defaults={"password": password}, | ||
) | ||
return UserDatabaseRoleMap.objects.create( |
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.
This throws up an IntegrityError
when we try to add the existing credentials via the API, I think we ought to use get_or_create
here as well, to ensure that we allow users to reinstall mathesar even if the database gets deleted outside of mathesar.
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'm okay with that change.
@Anish9901 Ready for re-review |
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.
Looks great @mathemancer, Thanks!
Fixes #3654
Fixes #3655
Fixes #3656
This adds
database_setup.create_new
,database_setup.connect_existing
, anddatabases.list
RPC functions.Technical details
Note that while this PR doesn't actually break anything, it does remove some functionality from the
connections
RPC functions. I think that's okay, since we'll be replacing those.All RPC functions are documented, and the documentation should be sufficient to understand them. If not, we need to improve the docs before merging this PR.
Screenshots
Checklist
Update index.md
).develop
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin