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

Add a contrib Dockerfile for local build image on Linux #4608

Merged
merged 5 commits into from
Jun 13, 2019

Conversation

agjohnson
Copy link
Contributor

This is necessary as permissions are all incorrect on the paths that are
shared between the host system and the Docker build container.

Closes #2692

@agjohnson agjohnson added this to the Development improvements milestone Sep 5, 2018
@agjohnson agjohnson added the Improvement Minor improvement to code label Sep 5, 2018
@agjohnson agjohnson requested a review from a team September 5, 2018 22:56
@agjohnson agjohnson mentioned this pull request Sep 6, 2018
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Good that finally, we are solving this issue in an automated way.

I think that there are still some cases to solve/document that I mentioned in the comments. I'd say that after that we can merge this PR.

Summarizing,

  • re tagging development images
  • defining setting for python versions

contrib/Dockerfile Outdated Show resolved Hide resolved
contrib/Dockerfile Outdated Show resolved Hide resolved
contrib/Dockerfile Outdated Show resolved Hide resolved
contrib/docker_build.sh Outdated Show resolved Hide resolved
docs/development/buildenvironments.rst Outdated Show resolved Hide resolved
contrib/Dockerfile Outdated Show resolved Hide resolved
@ericholscher
Copy link
Member

Anything we can do to move this along? Seems like a useful improvement.

@humitos
Copy link
Member

humitos commented Jan 9, 2019

It would be good to finish this PR and merge it. I think it's close.

@stale
Copy link

stale bot commented Mar 9, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Mar 9, 2019
@stale stale bot closed this Mar 16, 2019
@agjohnson agjohnson reopened this Mar 20, 2019
@stale stale bot removed the Status: stale Issue will be considered inactive soon label Mar 20, 2019
@agjohnson agjohnson added the Accepted Accepted issue on our roadmap label Mar 20, 2019
@codecov
Copy link

codecov bot commented Mar 20, 2019

Codecov Report

Merging #4608 into master will increase coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4608      +/-   ##
==========================================
+ Coverage   78.42%   78.54%   +0.11%     
==========================================
  Files         173      164       -9     
  Lines       10692    10089     -603     
  Branches     1357     1279      -78     
==========================================
- Hits         8385     7924     -461     
+ Misses       1958     1831     -127     
+ Partials      349      334      -15
Impacted Files Coverage Δ
readthedocs/search/views.py 78.94% <0%> (-11.97%) ⬇️
readthedocs/analytics/utils.py 42.85% <0%> (-9.78%) ⬇️
readthedocs/builds/syncers.py 33.33% <0%> (-5.23%) ⬇️
readthedocs/projects/version_handling.py 91.48% <0%> (-4.26%) ⬇️
readthedocs/core/views/serve.py 85.81% <0%> (-3.08%) ⬇️
readthedocs/builds/models.py 79.47% <0%> (-3.06%) ⬇️
readthedocs/sphinx_domains/models.py 75% <0%> (-1.67%) ⬇️
readthedocs/gold/views.py 65.67% <0%> (-1.48%) ⬇️
readthedocs/projects/views/private.py 80.62% <0%> (-1.16%) ⬇️
readthedocs/gold/models.py 95.65% <0%> (-1.02%) ⬇️
... and 73 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edf2e60...22b53d0. Read the comment docs.

agjohnson and others added 2 commits May 17, 2019 14:24
This is necessary as permissions are all incorrect on the paths that are
shared between the host system and the Docker build container.

Closes #2692
@humitos
Copy link
Member

humitos commented May 17, 2019

I rebased this branch and push a new commit to always use latest to build the image and also guide the user to re-tag the Docker image after building it so Read the Docs uses it without changing any setting that could break in other places.

@humitos humitos requested a review from a team May 17, 2019 12:52
contrib/Dockerfile Outdated Show resolved Hide resolved
contrib/Dockerfile Outdated Show resolved Hide resolved
This re-reverts back to using label for some of the dockerfile. Also,
instead of creating a group, we simply groupmod the existing docs group.

This doesn't address the configuration changes necessary for development
images yet.
@agjohnson
Copy link
Contributor Author

I've updated this pr to use a configurable label in the docker script. The last piece is how to handle DOCKER_IMAGE_SETTINGS, and I think the answer is maybe alter this setting conditionally to point to the dev images.

Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

Looks good!

contrib/Dockerfile Show resolved Hide resolved
contrib/docker_build.sh Outdated Show resolved Hide resolved
contrib/readme.rst Outdated Show resolved Hide resolved
…opment

This setting can be used from local_settings.py, and signifies that you
have run ``docker_build.sh`` to build local development images that
replace the UID/GID.
@agjohnson
Copy link
Contributor Author

I added a quick hack to settings.dev to allow for manual triggering of the docker image name replacement. One last review and I think this is ready.

@agjohnson agjohnson requested a review from humitos June 11, 2019 21:56
Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

Automatic enough

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

👍

@agjohnson
Copy link
Contributor Author

Let's see what new and interesting problems this will likely create for us eventually! 🤞

@agjohnson agjohnson merged commit 9dbca65 into master Jun 13, 2019
@delete-merged-branch delete-merged-branch bot deleted the agj/add-local-dev-docker branch June 13, 2019 19:41
@humitos humitos mentioned this pull request Oct 16, 2019
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants