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

Docker: install requirements from local changes #7409

Merged
merged 3 commits into from Aug 26, 2020
Merged

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Aug 25, 2020

This requires readthedocs/common#68

@stsewd stsewd requested a review from a team Aug 25, 2020
Copy link
Member

@humitos humitos left a comment

I don't like this change, and I'd prefer if we don't merge it. With this, we will end up with docker images that have random requirements installed on them instead of the latest from our master branches. I used curl on purpose here for this reason.

If you want to temporarily install requirements from the current branch you are working on, you should use entrypoints/common.sh script and add pip install -r requirements.txt to match the requirements of the branch you are working on.

@stsewd
Copy link
Member Author

stsewd commented Aug 25, 2020

What's the use case if you have local changes in the requirements files and don't want to install them? You'll be also installing them only if you rebuild the images.

@stsewd stsewd requested a review from a team Aug 25, 2020
@stsewd
Copy link
Member Author

stsewd commented Aug 25, 2020

And btw, using the scripts in common won't get you a clean environment, as some dependencies may be removed in the new requirements, so I don't think that's a valid reason to block this change.

@humitos
Copy link
Member

humitos commented Aug 26, 2020

What's the use case if you have local changes in the requirements files and don't want to install them? You'll be also installing them only if you rebuild the images.

My idea behind using curl was to all have the same images, no matter the status of your local copy. Then if you need any modification to the image, you can use the common.sh script or similar flow to install/update/delete packages or others. I think this avoid mistakes of having Docker images with corrupted data.

And btw, using the scripts in common won't get you a clean environment, as some dependencies may be removed in the new requirements, so I don't think that's a valid reason to block this change.

This is a good point.

Overall, I thought more about this and I don't think it's too important really. Yesterday I saw it more dangerous/error prone than it is in reality.

@humitos
Copy link
Member

humitos commented Aug 26, 2020

This requires readthedocs/common#68

Shouldn't this PR updates common/ submodule to point to that particular commit?

@stsewd stsewd merged commit 0e47034 into master Aug 26, 2020
2 checks passed
@stsewd stsewd deleted the docker-local-req branch Aug 26, 2020
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.

None yet

2 participants