Added RTD_VERSION_SLUG environment variable.

[rrahn]

Adds environment variable called RTD_VERSION_SLUG to identify selected version in conf.py.
See #1096 for more details.

[gregmuellegger]

I'll leave the review up to @agjohnson who knows the infrastructure in that place best.

[agjohnson]

This is only implemented on the BuildCommand class, it needs to be implemented on the DockerBuildCommand class as well.

We expose a READTHEDOCS var, this var should probably be called READTHEDOCS_VERSION, instead of RTD_VERSION_SLUG for consistency.

[ericholscher]

Sounds like we should have a "Add env variables" function that is shared between both Command classes?

[agjohnson]

Possibly. There is an added restriction here because env vars are added to the Popen call inside the BuildCommand class, whereas the docker API doesn't allow passing in env vars per command when using exec_create/exec_start. This would have to be done on environment creation, inside DockerBuildEnvironment, on the create_container call to the docker api:
https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/doc_builder/environments.py#L571-L587

See also:
http://docker-py.readthedocs.org/en/latest/api/#create_container

[rrahn]

Ok, I will change the name of the environment variable and add it to the DockerBuildCommand as described above.

[ericholscher]

Seems good. The only change I would say is make it READTHEDOCS_VERSION instead of RTD_VERSION_SLUG. 👍 on merging after that. Might also make sense to add a READTHEDOCS_PROJECT in there as well, for completeness.

[rrahn]

Ok I don't get the last comment correctly? I now should omit to pass the environment variable to the DockerBuilder and only rename it?
Otherwise, if I understand it correctly I have to pass the env variable to the DockerEnvironment when invoking create_container. Here I get the version slug from the base class BuildEnvironment which is also passed as build_env to the BuildCommand? Or where do I have to look for the version slug within the DockerEnvironment class?

[rrahn]

Ok I updated the PR. Hope this is done correctly. I am not so the python guy ... yet 😄

[ericholscher]

This isn't needed. The top part is the only bit that will run in prod.

Remove these lines, and I'll merge.

[rrahn]

Ok, this was the thing that confused me if I should still provide this to the DockerEnvironment.

[agjohnson]

This will effectively do nothing now that we are running build environments through Docker. My last comment above still stands, this will in fact need to be established on Docker container creation as well.

[ericholscher]

This is where I added the READTHEDOCS env stuff -- seems like this should work the same way?

[agjohnson]

It does work in the same way -- that is, it doesn't work :)

The above env pieces are only added on the BuildCommand call to Popen, the DockerEnvironment command execution doesn't use Popen at all, instead calling DockerBuildCommand.run, which executes through the Docker API via exec_start instead.

These pieces should be added to DockerEnvironment.create_container as well.

[rrahn]

Ok i pushed the changes to the DockerEnvironment.

[agjohnson]

Great, thanks!

[rrahn]

Oh cool.
Just one last question, are these changes already live or do we have to wait for the next release of RTD?
Thank you very much for your excellent support.

[agjohnson]

Not yet deployed, but we'll likely squeeze those deploys in later today.