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

Easier docker setup for development environment #1549

Closed
wants to merge 1 commit into from

Conversation

lullis
Copy link
Contributor

@lullis lullis commented Jan 30, 2025

I couldn't find anything in the documentation indicating how to set up the docker service for development. The Dockerfile is outdated (the second target is setup to run the idp project, but the env variables defined are not matching) and not really reflecting the current tooling used elsewhere. The docker-compose also does not have any mechanism to set an environment file which makes it hard to make changes.

Fixes #1548

Description of the Change

The Dockerfile now is used only to give a container with all the installed dependencies, and the docker-compose is focused on a development workflow, the application code is loaded as a volume, so changes in the code repository are immediately available inside the container. Environment variables that are specific to the development environment are defined in the dockerc-compose file, and environment variables that might be controlled by the developer (e.g, whether to enable or disable OIDC), can be configured through a .env file placed at the root of the project.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@lullis lullis requested a review from dopry January 31, 2025 03:34
@lullis lullis changed the title Fix Dockerfile Easier docker setup for development environment Jan 31, 2025
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.24%. Comparing base (e34819a) to head (279d875).
Report is 28 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1549      +/-   ##
==========================================
- Coverage   97.41%   97.24%   -0.17%     
==========================================
  Files          34       34              
  Lines        2164     2214      +50     
==========================================
+ Hits         2108     2153      +45     
- Misses         56       61       +5     
Flag Coverage Δ
?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- Dockerfile now only is concerned with building the container.
  Everything else (volumes, port configuration, environment variables
  for the application) is defined via compose
- Add a sample env file to allow easy configuration of different setups during development.
- Moved dependencies defined on requirements.txt from idp folder into dev dependencies from pyproject
- Added documentation to set up those services with Docker
@lullis
Copy link
Contributor Author

lullis commented Jan 31, 2025

Looks like the RP javascript app also needs to be changed.

@dopry
Copy link
Contributor

dopry commented Jan 31, 2025

I appreciate the contribution, but some of your changes are counter to our intent for the docker compose environment. The /tests/app/idp is a proof of concept we're working on as a potential app that represents a best practice DOT IDP that people can run in their own containerized infrastructure. It's not meant to be a 'development' container. The changed you've made here would make it impossible to distribute in the future.

The compose environment itself is mostly for maintainers and testers to stand up an environment quickly for QA and Pull Request code review.

Can you clarify what issues you had starting the environment initially and what you mean by

the env variables defined are not matching

@dopry
Copy link
Contributor

dopry commented Jan 31, 2025

I'm closing since #1550 was merged and should resolve #1548. I appreciate the work on this. You've presented some interesting ideas that are outside the scope of the issue, that I think warrant discussion. In particular a more mature package manager and a development container. Feel free to open issues for each of them independently as DX improvements and demonstrate how they're make our maintainers' and contributors' lives easier.

@dopry dopry closed this Jan 31, 2025
@lullis
Copy link
Contributor Author

lullis commented Jan 31, 2025

The compose environment itself is mostly for maintainers and testers to stand up an environment quickly for QA and Pull Request code review.

That is reasonable, but the question I have now is "how are people developing against this in the first place?"

Should I just create a separate project and install DOT in editable mode next to it?

@lullis lullis deleted the fix_dockerfile branch February 1, 2025 00:21
@dopry
Copy link
Contributor

dopry commented Feb 1, 2025

By running the idp locally, not in the container

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.

Dockerfile for idp test app not using correct paths
2 participants