Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 13, 2019
Merged

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

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
Closed
humitos
humitos requested changes Sep 6, 2018
View reviewed changes
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

@humitos humitos mentioned this pull request Sep 6, 2018
Merged
@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 •
edited
Loading

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.

@ericholscher ericholscher mentioned this pull request Mar 22, 2019
Closed
@humitos humitos mentioned this pull request Apr 1, 2019
Closed
agjohnson and others added 2 commits May 17, 2019 14:24
@agjohnson @humitos
Add a contrib Dockerfile for local build image on Linux
240fe78 
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
Use always latest image to build the Docker image
22b53d0
@humitos humitos force-pushed the agj/add-local-dev-docker branch from c3f9aad to 22b53d0 Compare May 17, 2019 12:49
@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
agjohnson
agjohnson commented Jun 6, 2019
View reviewed changes
@agjohnson
Make image label configurable in docker build script
6ad9930 
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.

stsewd
stsewd reviewed Jun 6, 2019
View reviewed changes
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!

agjohnson added 2 commits June 11, 2019 15:26
@agjohnson
Update command call for docker_build.sh
16c5ac3
@agjohnson
Add configuration option that patches the docker image names in devel…
3d9a84c 
…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
stsewd
stsewd approved these changes Jun 11, 2019
View reviewed changes
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

humitos
humitos approved these changes Jun 12, 2019
View reviewed changes
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
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@humitos humitos humitos approved these changes

@stsewd stsewd stsewd approved these changes

Assignees

@agjohnson agjohnson

Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
None yet
Milestone
Development improvements
Development

Successfully merging this pull request may close these issues.
4 participants
@agjohnson @ericholscher @humitos @stsewd