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

Merged
merged 2 commits into from Sep 6, 2018
Merged

Add cwd to subprocess calls #4611

merged 2 commits into from Sep 6, 2018

Conversation

agjohnson
Copy link
Contributor

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.

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 a team September 6, 2018 06:13
@agjohnson agjohnson added the Improvement Minor improvement to code label Sep 6, 2018
@agjohnson agjohnson added this to the Development improvements milestone 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.

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
Copy link
Contributor Author

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
Copy link
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
Copy link
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
Copy link
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
Copy link
Contributor Author

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

@humitos humitos merged commit 04901b3 into master Sep 6, 2018
@humitos humitos deleted the agj/add-cwd branch September 6, 2018 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants