Skip to content
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

MDBF-424: fix missing grant and zoneinfo for tests #6

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

an3l
Copy link
Contributor

@an3l an3l commented May 30, 2022

$ ./manage.py test
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
.......
----------------------------------------------------------------------
Ran 7 tests in 0.421s

OK
Destroying test database for alias 'default'...

- Check the issue 5 for problem statment
- After the patch
```
$ ./manage.py test
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
.......
----------------------------------------------------------------------
Ran 7 tests in 0.421s

OK
Destroying test database for alias 'default'...
```
@an3l an3l changed the title Issue 5: fix missing grant and zoneinfo for tests MDBF-424: fix missing grant and zoneinfo for tests May 30, 2022
Copy link
Member

@cvicentiu cvicentiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Anel!
You bring up a good point in that users might expect to run this code with Python versions lower than 3.9. However, changing the code to account for this is not something desirable. We are not going to run the code in production on anything that is not specified in the docker file and the code will not get tested in an different environment either.

As such, this ZoneInfo change is guaranteed to remain untested. I do not want to have this in the code base. Thus we should update the README file to properly set those expectations.

There is a reason why the current dev setup requires docker-compose. This ensures a uniform environment for all developers as well as the same environment that will be used in production.

The grant alter command is also wrong. The reason why you got the error in the first place is because you had a local MariaDB database instance when you ran the tests locally, not within the docker container. And that local database didn't grant the complete rights, like create_database_user.py does. Notice the next command at
cec59ac#diff-66a42c0ee250b38d7c0c694d6b7f820a8a1788000325f90b319405985249505dR45

This one grants rights on test_{} database, rights which imply ALTER.
The reason why CREATE is granted on the global level is for the user to be able to run CREATE DATABASE test_.... command

Thus, please change the PR to only document this intended workflow. Optionally, after PR #5 gets merged, one can document which variables to set to configure DJANGO to connect to another database, not the one running inside the container.

@an3l
Copy link
Contributor Author

an3l commented May 31, 2022

Hi @cvicentiu,
thanks for looking into this.
I have some questions and remarks:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants