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

Update readthedocs-environment.json file when env vars are added/deleted #5540

Merged
merged 14 commits into from
Apr 22, 2019
36 changes: 36 additions & 0 deletions readthedocs/doc_builder/python_environments.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"""An abstraction over virtualenv and Conda environments."""

import copy
import hashlib
import itertools
import json
import logging
Expand Down Expand Up @@ -149,6 +150,7 @@ def is_obsolete(self):
* the Python version (e.g. 2.7, 3, 3.6, etc)
* the Docker image name
* the Docker image hash
* the environment variables hash

:returns: ``True`` when it's obsolete and ``False`` otherwise

Expand All @@ -174,6 +176,7 @@ def is_obsolete(self):

env_python = environment_conf.get('python', {})
env_build = environment_conf.get('build', {})
env_vars_hash = environment_conf.get('env_vars_hash', None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defaulting to None will cause to every build before this change to wipe. Not really sure if that will be a problem, but we could default to the hash of an empty string here to prevent that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeahhh.. that is better.
Should I generate it every time or should I copy the value from the web for the sha256 hash of empty string?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defaulting to None is fine.

The internal method should return None as well if there are no Environment Variables.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The internal method returns the hash of the empty string

Copy link
Member Author

@dojutsu-user dojutsu-user Apr 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@humitos @stsewd
What should we do here?
I am agreeing with @stsewd idea here. Returning the hash of empty string saves us few lines of code.


# By defaulting non-existent options to ``None`` we force a wipe since
# we don't know how the environment was created
Expand All @@ -197,8 +200,39 @@ def is_obsolete(self):
env_python_version != self.config.python_full_version,
env_build_image != build_image,
env_build_hash != image_hash,
env_vars_hash != self._get_env_vars_hash(),
])

def _get_env_vars(self):
"""
Returns environment variables with their values of the associated project.

If there are no environment variables, it returns None.
"""
env_vars = self.version.project.environmentvariable_set.values_list('name', 'value')
dojutsu-user marked this conversation as resolved.
Show resolved Hide resolved
if env_vars.exists():
return tuple(env_vars)
dojutsu-user marked this conversation as resolved.
Show resolved Hide resolved

def _get_env_vars_hash(self):
"""
Returns the sha256 hash of all the environment variables and their values.

If there are no environment variables, it returns the sha256 hash of 'None'.
"""
env_vars = self._get_env_vars()
m = hashlib.sha256()
if env_vars:
for env_var in env_vars:
dojutsu-user marked this conversation as resolved.
Show resolved Hide resolved
hash_str = '_{var}_{value}_'.format(
var=env_var[0],
value=env_var[1]
)
m.update(hash_str.encode('utf-8'))
else:
m.update('None'.encode('utf-8'))
dojutsu-user marked this conversation as resolved.
Show resolved Hide resolved

return m.hexdigest()

def save_environment_json(self):
"""
Save on builders disk data about the environment used to build docs.
Expand All @@ -208,11 +242,13 @@ def save_environment_json(self):
- python.version
- build.image
- build.hash
- env_vars_hash
"""
data = {
'python': {
'version': self.config.python_full_version,
},
'env_vars_hash': self._get_env_vars_hash(),
}

if isinstance(self.build_env, DockerBuildEnvironment):
Expand Down
22 changes: 20 additions & 2 deletions readthedocs/rtd_tests/tests/test_doc_building.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* raw subprocess calls like .communicate expects bytes
* the Command wrappers encapsulate the bytes and expose unicode
"""
import hashlib
import json
import os
import re
Expand All @@ -30,7 +31,7 @@
)
from readthedocs.doc_builder.exceptions import BuildEnvironmentError
from readthedocs.doc_builder.python_environments import Conda, Virtualenv
from readthedocs.projects.models import Project
from readthedocs.projects.models import Project, EnvironmentVariable
from readthedocs.rtd_tests.mocks.environment import EnvironmentMockGroup
from readthedocs.rtd_tests.mocks.paths import fake_paths_lookup
from readthedocs.rtd_tests.tests.test_config_integration import create_load
Expand Down Expand Up @@ -1469,13 +1470,21 @@ def test_save_environment_json(self, load_config):
config=config,
)

self.assertFalse(self.pip.environmentvariable_set.all().exists())
get(EnvironmentVariable, project=self.pip, name='ABCD', value='1234')
env_var_str = '_ABCD_1234_'
m = hashlib.sha256()
m.update(env_var_str.encode('utf-8'))
env_vars_hash = m.hexdigest()

with patch(
'readthedocs.doc_builder.python_environments.PythonEnvironment.environment_json_path',
return_value=tempfile.mktemp(suffix='envjson'),
):
python_env.save_environment_json()
json_data = json.load(open(python_env.environment_json_path()))


expected_data = {
'build': {
'image': 'readthedocs/build:2.0',
Expand All @@ -1484,6 +1493,7 @@ def test_save_environment_json(self, load_config):
'python': {
'version': 2.7,
},
'env_vars_hash': env_vars_hash
}
self.assertDictEqual(json_data, expected_data)

Expand Down Expand Up @@ -1610,7 +1620,15 @@ def test_is_obsolete_with_json_same_data_as_version(self, load_config):
build_env=self.build_env,
config=config,
)
env_json_data = '{"build": {"image": "readthedocs/build:2.0", "hash": "a1b2c3"}, "python": {"version": 3.5}}' # noqa

self.assertFalse(self.pip.environmentvariable_set.all().exists())
get(EnvironmentVariable, project=self.pip, name='HELLO', value='WORLD')
env_var_str = '_HELLO_WORLD_'
m = hashlib.sha256()
m.update(env_var_str.encode('utf-8'))
env_vars_hash = m.hexdigest()

env_json_data = '{{"build": {{"image": "readthedocs/build:2.0", "hash": "a1b2c3"}}, "python": {{"version": 3.5}}, "env_vars_hash": "{}"}}'.format(env_vars_hash) # noqa
with patch('os.path.exists') as exists, patch('readthedocs.doc_builder.python_environments.open', mock_open(read_data=env_json_data)) as _open: # noqa
exists.return_value = True
self.assertFalse(python_env.is_obsolete)
Expand Down