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

Fix #1817 docker dx #1828

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

pbratkowski
Copy link
Contributor

@pbratkowski pbratkowski commented Dec 16, 2024

GitHub closed my previous PR (#1819) after I renamed a branch, so here is a new one.

Thanks to @tobiasmcnulty for pointing out a flaw in my previous implementation. This uses a build argument to determine when to remove packages, which allows removing them in the production build, but keeping them during local development which allows tox to build psycopg successfully in docker.

This also adds a container for the Trac database, which eliminates some errors upon startup, and makes tests pass in docker in most situations, and eliminates a lot of redundancy from the Docker settings file.

Fixes #1817

@pbratkowski
Copy link
Contributor Author

Tests currently still fail if PARENT_HOST is set to a non-default value, e.g. if running on a LAN rather than on localhost.

The issue with those is that they use hardcoded domains, e.g.

def test_sitemap_index(self):
response = self.client.get(
"/sitemap.xml", headers={"host": "docs.djangoproject.localhost:8000"}
)
self.assertContains(response, "<sitemap>", count=2)
self.assertContains(
response,
"<loc>http://docs.djangoproject.localhost:8000/sitemap-en.xml</loc>",
)

Replacing the hardcoded values with something like the following makes them pass successfully:

    def test_sitemap_index(self):
        response = self.client.get(
            "/sitemap.xml", headers={"host": f"docs.{settings.PARENT_HOST}"}
        )
        self.assertContains(response, "<sitemap>", count=2)
        self.assertContains(
            response,
            f"<loc>http://docs.{settings.PARENT_HOST}/sitemap-en.xml</loc>",
        )

If anyone has alternative solutions, let me know.

@pbratkowski pbratkowski marked this pull request as ready for review December 16, 2024 12:06
Copy link
Member

@pauloxnet pauloxnet left a comment

Choose a reason for hiding this comment

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

What's the value in keeping the dependencies in Docker?
I see mixed changes in this PR (e.g. dockers, settings.PARENT_HOST, ...)
Can you provide seprataed PRs ?

Dockerfile Outdated Show resolved Hide resolved
@pbratkowski
Copy link
Contributor Author

pbratkowski commented Dec 16, 2024

What's the value in keeping the dependencies in Docker?

Sorry, I should have added it to the description (which I have now updated). As mentioned in #1817 tox fails to install psycopg in Docker when the build dependencies are purged. By setting the argument in docker-compose.yml, this allows running tests in local dev environments.

As per django#1817, tox is missing build dependencies and cannot run tests
in containers. This commit added an argument to make purging packages
optional, allowing to optimize image for deployment, while also
keeping the required packages in dev environments.
Added 'tracdb' container to compose file. Simplified docker settings to
remove duplication. Added support for 'secrets.json' in the docker dev
stage. Added the 'DJANGOPROJECT_DATA_DIR' env var to tox.
@pbratkowski pbratkowski force-pushed the feat/docker-dx branch 2 times, most recently from b1f566e to 65b2438 Compare December 17, 2024 08:01
@pbratkowski
Copy link
Contributor Author

@pauloxnet how granular would you like the changes to be? I have removed the commit that fixes tests when settings.PARENT_HOST is set.

@pbratkowski pbratkowski changed the title Feat/docker dx Fix #1817 docker dx Dec 17, 2024
@pbratkowski pbratkowski marked this pull request as draft December 19, 2024 14:38
@pbratkowski
Copy link
Contributor Author

I converted this back to a Draft for now, as we are working out smaller scope PRs.

@marksweb marksweb self-assigned this Dec 22, 2024
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.

Instructions for running tox with docker don't work
3 participants