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

Vendor django-automated-logging #1794

Merged
merged 8 commits into from
Jul 19, 2023

Conversation

bmclaughlin
Copy link
Contributor

@bmclaughlin bmclaughlin commented Jul 12, 2023

What is this PR doing:

The django-automated-logging module we use for api access logging hasn't been touched in ~2 years. It is not Django 4.x compatible. A decision was made to vendor the code to maintain current functionality and not block our Django 4 upgrade.

I'm in the process of determining the best way to verify the logging in tests, currently planned for a follow-up PR.

Issue: AAH-2388

Reviewers must know:

When reviewing/testing in oci-env, PULP_GALAXY_ENABLE_API_ACCESS_LOG=true must be set in the environment and the resulting log can be inspected with the following commands:

oci-env shell
tail -f /var/log/galaxy_api_access.log

Navigating around the app should log CRUD actions as well as username, ip address, and other details. Full details are available in the original jira epic https://issues.redhat.com/browse/AAH-643.

@github-actions github-actions bot added backport-4.2 This PR should be backported to stable-4.2 (1.2) backport-4.4 This PR should be backported to stable-4.4 (2.1) backport-4.5 This PR should be backported to stable-4.5 (2.2) backport-4.6 This PR should be backported to stable-4.6 (2.3) backport-4.7 This PR should be backported to stable-4.7 (2.4) labels Jul 12, 2023
@bmclaughlin bmclaughlin removed backport-4.2 This PR should be backported to stable-4.2 (1.2) backport-4.4 This PR should be backported to stable-4.4 (2.1) backport-4.5 This PR should be backported to stable-4.5 (2.2) backport-4.6 This PR should be backported to stable-4.6 (2.3) backport-4.7 This PR should be backported to stable-4.7 (2.4) labels Jul 12, 2023
@bmclaughlin bmclaughlin force-pushed the vendor-django-automated-logging branch 4 times, most recently from 404f7bf to aec6431 Compare July 14, 2023 19:25
@rochacbruno
Copy link
Member

Instead of dealing with setup.py and packaging I would simply copy automated_logging to galaxy_ng/ and then on INSTALLED_APPS add galaxy_ng.automated_logging - if there are internal absolute imports looking for from automated_logging etc.. then we can force it to sys.modules on galaxy_ng __init__.py

import sys
import automated_logging
sys.modules.setdefault('automated_logging', automated_logging)

@bmclaughlin bmclaughlin force-pushed the vendor-django-automated-logging branch 4 times, most recently from 6d033ce to 265b854 Compare July 18, 2023 19:35
@bmclaughlin bmclaughlin marked this pull request as ready for review July 18, 2023 20:20
@cutwater
Copy link
Contributor

Minor: It would be nice if the vendored package is placed in a subpackage (e.g. galaxy_ng._vendor.automated_logging)

@bmclaughlin bmclaughlin force-pushed the vendor-django-automated-logging branch from 265b854 to d57cfb5 Compare July 19, 2023 13:13
@bmclaughlin
Copy link
Contributor Author

/retest

1 similar comment
@bmclaughlin
Copy link
Contributor Author

/retest

Issue: AAH-2388
@bmclaughlin
Copy link
Contributor Author

bmclaughlin commented Jul 19, 2023

@cutwater Moved to a subpackage as suggested.

@jctanner jctanner merged commit 11b5926 into ansible:master Jul 19, 2023
16 of 17 checks passed
@jctanner
Copy link
Collaborator

Tests will be added in a future PR.

@bmclaughlin bmclaughlin deleted the vendor-django-automated-logging branch July 19, 2023 23:06
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.

4 participants