-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Use BuildConfig model in our code
#12675
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: humitos <[email protected]>
Co-authored-by: humitos <[email protected]>
… basic Django tests Co-authored-by: humitos <[email protected]>
…pilot/simplify-config-attribute-model
…mitos/use-buildconfig-model
We can remove this method in the future if we want, but I'm keeping it for backwards compatibility for now since I wasn't able to change it.
ericholscher
left a comment
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.
Would be good to have @stsewd review as well, but did a quick look.
|
@stsewd waiting on review from you to 👍 it |
stsewd
left a comment
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 should change how the index is created #12704 before switching to this new modeling. Otherwise, it would be harder to change later.
| ) | ||
| .order_by("-date") | ||
| .only("_config") | ||
| .only("readthedocs_yaml_config") |
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 was used because we needed to query other builds. You can change this to just values_list("readthedocs_yaml_config", flat=True)
|
|
||
| def save(self, *args, **kwargs): # noqa | ||
| if self._readthedocs_yaml_config_changed: | ||
| build_config, _ = BuildConfig.objects.get_or_create(data=self._readthedocs_yaml_config) |
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.
_readthedocs_yaml_config could have been assigned to None. In that case we won't have a build_config object.
I'm fine moving forward without that index for now. That was a test and it's not strictly required. I can do it later if it's required. It shouldn't be a lot of data. We have created 500k |
|
I'd prefer if we deal with that now and not when the DB is overloaded in the future. |
This is the continuation work to finally make usage of the
BuildConfigmodel instead of saving thereadthedocs.yamlfile insideBuild._configitself.The next step here is to delete the
_configfield once we are settled here and happy with this solution 👍🏼References:
data_hashfield and updateget_or_createmethod #12704