-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Save config on build model #4749
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
Changes from all commits
6891db8
b1b3620
27d8251
ab5ea04
5f4fb8c
9d70812
13abeda
98d26a4
660c03a
d29b6d9
7fc1e2b
8f62f03
76929ac
7ade872
6f8becc
07efa1b
715e5ed
dab5860
5aacd63
822b793
8c60760
7ed110d
2ffdbf6
5cce5d8
9d8266f
28cd935
433b709
b88837d
4723705
043f4e6
79fbb8e
f33d54a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
# -*- coding: utf-8 -*- | ||
# Generated by Django 1.11.16 on 2018-11-02 13:24 | ||
from __future__ import unicode_literals | ||
|
||
from django.db import migrations | ||
import jsonfield.fields | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('builds', '0005_remove-version-alias'), | ||
] | ||
|
||
operations = [ | ||
migrations.AddField( | ||
model_name='build', | ||
name='_config', | ||
field=jsonfield.fields.JSONField(default=dict, verbose_name='Configuration used in the build'), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,10 +17,12 @@ | |
from django.conf import settings | ||
from django.core.urlresolvers import reverse | ||
from django.db import models | ||
from django.utils import timezone | ||
from django.utils.encoding import python_2_unicode_compatible | ||
from django.utils.translation import ugettext | ||
from django.utils.translation import ugettext_lazy as _ | ||
from guardian.shortcuts import assign | ||
from jsonfield import JSONField | ||
from taggit.managers import TaggableManager | ||
|
||
from readthedocs.core.utils import broadcast | ||
|
@@ -128,6 +130,21 @@ def __str__(self): | |
pk=self.pk, | ||
)) | ||
|
||
@property | ||
def config(self): | ||
""" | ||
Proxy to the configuration of the build. | ||
|
||
:returns: The configuration used in the last successful build. | ||
:rtype: dict | ||
""" | ||
last_build = ( | ||
self.builds.filter(state='finished', success=True) | ||
.order_by('-date') | ||
.first() | ||
) | ||
return last_build.config | ||
stsewd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
@property | ||
def commit_name(self): | ||
""" | ||
|
@@ -450,6 +467,7 @@ class Build(models.Model): | |
exit_code = models.IntegerField(_('Exit code'), null=True, blank=True) | ||
commit = models.CharField( | ||
_('Commit'), max_length=255, null=True, blank=True) | ||
_config = JSONField(_('Configuration used in the build'), default=dict) | ||
|
||
length = models.IntegerField(_('Build Length'), null=True, blank=True) | ||
|
||
|
@@ -463,11 +481,81 @@ class Build(models.Model): | |
|
||
objects = BuildQuerySet.as_manager() | ||
|
||
CONFIG_KEY = '__config' | ||
|
||
class Meta(object): | ||
ordering = ['-date'] | ||
get_latest_by = 'date' | ||
index_together = [['version', 'state', 'type']] | ||
|
||
def __init__(self, *args, **kwargs): | ||
super(Build, self).__init__(*args, **kwargs) | ||
self._config_changed = False | ||
|
||
@property | ||
def previous(self): | ||
""" | ||
Returns the previous build to the current one. | ||
|
||
Matching the project and version. | ||
""" | ||
date = self.date or timezone.now() | ||
if self.project is not None and self.version is not None: | ||
return ( | ||
Build.objects | ||
.filter( | ||
project=self.project, | ||
version=self.version, | ||
date__lt=date, | ||
) | ||
.order_by('-date') | ||
.first() | ||
) | ||
return None | ||
|
||
@property | ||
def config(self): | ||
""" | ||
Get the config used for this build. | ||
|
||
Since we are saving the config into the JSON field only when it differs | ||
from the previous one, this helper returns the correct JSON used in | ||
this Build object (it could be stored in this object or one of the | ||
previous ones). | ||
""" | ||
if self.CONFIG_KEY in self._config: | ||
return Build.objects.get(pk=self._config[self.CONFIG_KEY])._config | ||
return self._config | ||
|
||
@config.setter | ||
def config(self, value): | ||
""" | ||
Set `_config` to value. | ||
|
||
`_config` should never be set directly from outside the class. | ||
""" | ||
self._config = value | ||
self._config_changed = True | ||
|
||
def save(self, *args, **kwargs): # noqa | ||
""" | ||
Save object. | ||
|
||
To save space on the db we only save the config if it's different | ||
from the previous one. | ||
|
||
If the config is the same, we save the pk of the object | ||
that has the **real** config under the `CONFIG_KEY` key. | ||
""" | ||
if self.pk is None or self._config_changed: | ||
stsewd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
previous = self.previous | ||
if (previous is not None and | ||
self._config and self._config == previous.config): | ||
previous_pk = previous._config.get(self.CONFIG_KEY, previous.pk) | ||
self._config = {self.CONFIG_KEY: previous_pk} | ||
super(Build, self).save(*args, **kwargs) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a little worried that we're running this on each Is there a way to set the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there is an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, I'm just trying to think if there is a way to run this logic just when we are trying to set a |
||
self._config_changed = False | ||
|
||
def __str__(self): | ||
return ugettext( | ||
'Build {project} for {usernames} ({pk})'.format( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -840,6 +840,60 @@ def test_config_filenames_regex(correct_config_filename): | |
assert re.match(CONFIG_FILENAME_REGEX, correct_config_filename) | ||
|
||
|
||
def test_as_dict(tmpdir): | ||
apply_fs(tmpdir, {'requirements.txt': ''}) | ||
build = get_build_config( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name of the variable is tricky. This is not a build object, but a config object. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. Feel free to do whatever you prefer. In case you think this should be renamed, open an issue in the Cleanup milestone for the "future". |
||
{ | ||
'version': 1, | ||
'formats': ['pdf'], | ||
'python': { | ||
'version': 3.5, | ||
}, | ||
'requirements_file': 'requirements.txt', | ||
}, | ||
get_env_config({ | ||
'defaults': { | ||
'doctype': 'sphinx', | ||
'sphinx_configuration': None, | ||
}, | ||
}), | ||
source_file=str(tmpdir.join('readthedocs.yml')), | ||
) | ||
build.validate() | ||
expected_dict = { | ||
'version': '1', | ||
'formats': ['pdf'], | ||
'python': { | ||
'version': 3.5, | ||
'requirements': 'requirements.txt', | ||
'install_with_pip': False, | ||
'install_with_setup': False, | ||
'extra_requirements': [], | ||
'use_system_site_packages': False, | ||
}, | ||
'build': { | ||
'image': 'readthedocs/build:2.0', | ||
}, | ||
'conda': None, | ||
'sphinx': { | ||
'builder': 'sphinx', | ||
'configuration': None, | ||
'fail_on_warning': False, | ||
}, | ||
'mkdocs': { | ||
'configuration': None, | ||
'fail_on_warning': False, | ||
}, | ||
'doctype': 'sphinx', | ||
'submodules': { | ||
'include': ALL, | ||
'exclude': [], | ||
'recursive': True, | ||
}, | ||
} | ||
assert build.as_dict() == expected_dict | ||
|
||
|
||
class TestBuildConfigV2(object): | ||
|
||
def get_build_config(self, config, env_config=None, | ||
|
@@ -1811,3 +1865,47 @@ def test_pop_config_raise_exception(self): | |
build.pop_config('one.four', raise_ex=True) | ||
assert excinfo.value.value == 'four' | ||
assert excinfo.value.code == VALUE_NOT_FOUND | ||
|
||
def test_as_dict(self, tmpdir): | ||
apply_fs(tmpdir, {'requirements.txt': ''}) | ||
build = self.get_build_config( | ||
{ | ||
'version': 2, | ||
'formats': ['pdf'], | ||
'python': { | ||
'version': 3.6, | ||
'requirements': 'requirements.txt', | ||
}, | ||
}, | ||
source_file=str(tmpdir.join('readthedocs.yml')), | ||
) | ||
build.validate() | ||
expected_dict = { | ||
'version': '2', | ||
'formats': ['pdf'], | ||
'python': { | ||
'version': 3.6, | ||
'requirements': str(tmpdir.join('requirements.txt')), | ||
'install_with_pip': False, | ||
'install_with_setup': False, | ||
'extra_requirements': [], | ||
'use_system_site_packages': False, | ||
}, | ||
'build': { | ||
'image': 'readthedocs/build:latest', | ||
}, | ||
'conda': None, | ||
'sphinx': { | ||
'builder': 'sphinx', | ||
'configuration': None, | ||
'fail_on_warning': False, | ||
}, | ||
'mkdocs': None, | ||
'doctype': 'sphinx', | ||
'submodules': { | ||
'include': [], | ||
'exclude': ALL, | ||
'recursive': False, | ||
}, | ||
} | ||
assert build.as_dict() == expected_dict |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -406,8 +406,10 @@ def run_setup(self, record=True): | |
raise YAMLParseError( | ||
YAMLParseError.GENERIC_WITH_PARSE_EXCEPTION.format( | ||
exception=str(e), | ||
)) | ||
) | ||
) | ||
|
||
self.save_build_config() | ||
self.additional_vcs_operations() | ||
|
||
if self.setup_env.failure or self.config is None: | ||
|
@@ -531,9 +533,14 @@ def get_build(build_pk): | |
build = {} | ||
if build_pk: | ||
build = api_v2.build(build_pk).get() | ||
return dict((key, val) for (key, val) in list(build.items()) | ||
if key not in ['project', 'version', 'resource_uri', | ||
'absolute_uri']) | ||
private_keys = [ | ||
'project', 'version', 'resource_uri', 'absolute_uri' | ||
] | ||
return { | ||
key: val | ||
for key, val in build.items() | ||
if key not in private_keys | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The blocks are the same, just refactored it to be more readable |
||
|
||
def setup_vcs(self): | ||
""" | ||
|
@@ -594,6 +601,15 @@ def set_valid_clone(self): | |
self.project.has_valid_clone = True | ||
self.version.project.has_valid_clone = True | ||
|
||
def save_build_config(self): | ||
"""Save config in the build object.""" | ||
pk = self.build['id'] | ||
config = self.config.as_dict() | ||
api_v2.build(pk).patch({ | ||
'config': config, | ||
}) | ||
self.build['config'] = config | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we are going to use this here, but just in case, it also helps when testing this, write a test for this was hard 😞 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, testing just about everything about our builds is tricky :/ |
||
|
||
def update_app_instances(self, html=False, localmedia=False, search=False, | ||
pdf=False, epub=False): | ||
""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,18 +112,23 @@ class BuildSerializer(serializers.ModelSerializer): | |
version_slug = serializers.ReadOnlyField(source='version.slug') | ||
docs_url = serializers.ReadOnlyField(source='version.get_absolute_url') | ||
state_display = serializers.ReadOnlyField(source='get_state_display') | ||
# Jsonfield needs an explicit serializer | ||
# https://github.com/dmkoch/django-jsonfield/issues/188#issuecomment-300439829 | ||
config = serializers.JSONField(required=False) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need this because of rpkilby/jsonfield#188 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably should be a code comment |
||
|
||
class Meta(object): | ||
model = Build | ||
exclude = ('builder',) | ||
# `_config` should be excluded to avoid conflicts with `config` | ||
exclude = ('builder', '_config') | ||
|
||
|
||
class BuildAdminSerializer(BuildSerializer): | ||
|
||
"""Build serializer for display to admin users and build instances.""" | ||
|
||
class Meta(BuildSerializer.Meta): | ||
exclude = () | ||
# `_config` should be excluded to avoid conflicts with `config` | ||
exclude = ('_config',) | ||
|
||
|
||
class SearchIndexSerializer(serializers.Serializer): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how long this query will take. Our build table is pretty large, but perhaps the filtering will make it a bit faster. Feels like something we might want to denormalize if we use it a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use this, for now, we'll see if we can cache this maybe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is only used in the tests. Do we need this at all? Did you add it as a helper for debugging or so? Does it worth to keep it here? Where do you think it will be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be used on the api and resolver, it'll be used on the second part of this PR