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 cwd to subprocess calls #4611

Merged
merged 2 commits into from Sep 6, 2018

Conversation

Projects
None yet
3 participants
@agjohnson
Contributor

agjohnson commented Sep 6, 2018

We were assuming this path before, which defaulted to the path you
executed django from on the host machine. The cwd was used in commands
run on inside Docker however. On Linux dev environments, this path is
most likely missing.

By specifying paths, we will get paths that have been created inside the
container.

Add cwd to subprocess calls
We were assuming this path before, which defaulted to the path you
executed django from on the host machine. The cwd was used in commands
run on inside Docker however. On Linux dev environments, this path is
most likely missing.

By specifying paths, we will get paths that have been created inside the
container.

@agjohnson agjohnson requested a review from rtfd/core Sep 6, 2018

@agjohnson agjohnson added this to the Development improvements milestone Sep 6, 2018

@humitos

I don't follow what this fixes. What's the error that you were getting without applying these changes?

I've never experienced this before in my local environment.

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Sep 6, 2018

Contributor

My instance is broken between all my patches, but the error something like this in the virtualenv step:

can't cd to /home/anthony/secure/dev/readthedocs-corporate

It's because the application uses the cwd of the application, which does not exist in the docker container. Instead of just using the default cwd for these commands, we specify one so that the host system cwd isn't used.

Not sure why you wouldn't have hit this, unless you added extra paths into docker or change a setting i didn't change.

Contributor

agjohnson commented Sep 6, 2018

My instance is broken between all my patches, but the error something like this in the virtualenv step:

can't cd to /home/anthony/secure/dev/readthedocs-corporate

It's because the application uses the cwd of the application, which does not exist in the docker container. Instead of just using the default cwd for these commands, we specify one so that the host system cwd isn't used.

Not sure why you wouldn't have hit this, unless you added extra paths into docker or change a setting i didn't change.

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Sep 6, 2018

Member

Not sure why you wouldn't have hit this, unless you added extra paths into docker or change a setting i didn't change.

I didn't change any setting related to this.

Maybe this is still a permission issue and since I'm creating my Docker image in a different way (#4608 (comment)) than you, I'm not experimenting this.

What about you, @stsewd?

Member

humitos commented Sep 6, 2018

Not sure why you wouldn't have hit this, unless you added extra paths into docker or change a setting i didn't change.

I didn't change any setting related to this.

Maybe this is still a permission issue and since I'm creating my Docker image in a different way (#4608 (comment)) than you, I'm not experimenting this.

What about you, @stsewd?

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Sep 6, 2018

Member

I haven't experiment this as well, I think that error is the same with the error permissions.

Member

stsewd commented Sep 6, 2018

I haven't experiment this as well, I think that error is the same with the error permissions.

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Sep 6, 2018

Member

Well, I don't know what's happening but after pulling latest master in both (.org and .com) I started having this issue and this PR solves this.

I tried by building in .com and after applying this patch it worked.

So, I don't understand why it wasn't happening before and it's happening now.

Member

humitos commented Sep 6, 2018

Well, I don't know what's happening but after pulling latest master in both (.org and .com) I started having this issue and this PR solves this.

I tried by building in .com and after applying this patch it worked.

So, I don't understand why it wasn't happening before and it's happening now.

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Sep 6, 2018

Contributor

Oh interesting. Perhaps we changed something with paths here lately.

Contributor

agjohnson commented Sep 6, 2018

Oh interesting. Perhaps we changed something with paths here lately.

@humitos humitos merged commit 04901b3 into master Sep 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@humitos humitos deleted the agj/add-cwd branch Sep 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment