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

Refactor settings checks using django.core.checks #1121

Closed
wants to merge 1 commit into from

Conversation

daviddavis
Copy link
Contributor

fixes #8077

Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/contributing/pull-request-walkthrough.html

@pulpbot
Copy link
Member

pulpbot commented Feb 11, 2021

Attached issue: https://pulp.plan.io/issues/8077

@daviddavis daviddavis force-pushed the issue8077 branch 5 times, most recently from 62b6b40 to 5cd1fad Compare February 11, 2021 19:16
# the E003 check will handle this problem. skip to prevent FieldError.
continue

if Artifact.objects.filter(**{checksum: None}).exists():
Copy link
Member

Choose a reason for hiding this comment

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

from the logs, it seems it is executed before the migrations, so it breaks because core_artifact is not on the database yet

Copy link
Member

Choose a reason for hiding this comment

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

I read, that this is taken care of in line 120.

@daviddavis daviddavis force-pushed the issue8077 branch 2 times, most recently from 9ab7dc6 to df4a498 Compare February 11, 2021 20:38
@@ -270,76 +262,3 @@
ENVVAR_FOR_DYNACONF="PULP_SETTINGS",
)
# HERE ENDS DYNACONF EXTENSION LOAD (No more code below this line)
Copy link
Member

Choose a reason for hiding this comment

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

Ha! Now it makes sence!

Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

Not an expert on django checks, but this looks reasonable.

@bmbouter
Copy link
Member

When I run pstop then pclean I get this error with this PR:

(pulp) [vagrant@pulp3-source-fedora33 docs]$ pclean
pstasystemctl stop pulpcore-content pulpcore-worker@1 pulpcore-worker@2 pulpcore-resource-manager pulpcore-api
rt
pulp [None]: root:INFO: Executing... "DROP DATABASE "pulp";"
Traceback (most recent call last):
  File "/usr/local/lib/pulp/bin/pulpcore-manager", line 33, in <module>
    sys.exit(load_entry_point('pulpcore', 'console_scripts', 'pulpcore-manager')())
  File "/home/vagrant/devel/pulpcore/pulpcore/app/manage.py", line 11, in manage
    execute_from_command_line(sys.argv)
  File "/usr/local/lib/pulp/lib64/python3.9/site-packages/django/core/management/__init__.py", line 381, in execute_from_command_line
    utility.execute()
  File "/usr/local/lib/pulp/lib64/python3.9/site-packages/django/core/management/__init__.py", line 375, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/local/lib/pulp/lib64/python3.9/site-packages/django/core/management/base.py", line 323, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/usr/local/lib/pulp/lib64/python3.9/site-packages/django/core/management/base.py", line 364, in execute
    output = self.handle(*args, **options)
  File "/usr/local/lib/pulp/lib64/python3.9/site-packages/django_extensions/management/utils.py", line 62, in inner
    ret = func(self, *args, **kwargs)
  File "/usr/local/lib/pulp/lib64/python3.9/site-packages/django_extensions/management/commands/reset_db.py", line 175, in handle
    cursor.execute(drop_query)
psycopg2.errors.ObjectInUse: database "pulp" is being accessed by other users
DETAIL:  There is 1 other session using the database.

@bmbouter
Copy link
Member

I can't remember what our motivation was to move these to be django.checks. I know that is probably a better mechanism, but also we've had problems using them for our assertions. Whenever I've tried to make a django.check I couldn't be sure it was running at startup for example.

@daviddavis
Copy link
Contributor Author

When I run pstop then pclean I get this error with this PR:

Ah, I tried to use the Artifact model instead of db cursor. Looks like I'l have to switch back to the cursor.

I can't remember what our motivation was to move these to be django.checks. I know that is probably a better mechanism, but also we've had problems using them for our assertions. Whenever I've tried to make a django.check I couldn't be sure it was running at startup for example.

It's definitely a better mechanism. It's more in line with what django recommends and it keeps business logic out of the settings file where it doesn't belong. I'd be interested in knowing more about the problems you had. For one thing, the storage checks currently in the file are deployment checks so they don't run automatically.

@bmbouter
Copy link
Member

It's definitely a better mechanism. It's more in line with what django recommends and it keeps business logic out of the settings file where it doesn't belong.

Agreed on all that; I just couldn't get it working when I did it (see below).

I'd be interested in knowing more about the problems you had. For one thing, the storage checks currently in the file are deployment checks so they don't run automatically.

It's been a few weeks, but iirc, even when I had a check that should fail in place, when I started each component, the processes still started without failure. I'll test this when you push yours next and if it indeed fails, then I think we're good to go.

@daviddavis daviddavis closed this Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants