-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Build: remove usage of addons field #11846
Conversation
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 can remove the field itself after deploying this PR.
Why you don't create a migration with Safe.after_deploy
that deletes the field?
@@ -304,7 +304,7 @@ def test_build_updates_documentation_type(self, load_yaml_config): | |||
assert self.requests_mock.request_history[8]._request.method == "PATCH" | |||
assert self.requests_mock.request_history[8].path == "/api/v2/version/1/" | |||
assert self.requests_mock.request_history[8].json() == { | |||
"addons": True, | |||
"addons": False, |
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 not even send the addons
fields on these API calls now.
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 is because we use a APIVersion
model that inherits from the Version model, it will be removed when the field is removed from the model.
Old code/tasks will try to use that field, but will error since it doesn't exist in the new code. |
I don't follow you. If the old code is executed, the field is there. If the new code is executed the field is not there. In both cases, the field is in the db. After the deploy, new code won't make usage of the field and that time, the migration will be executed and the field deleted from the db. I'm not seeing where is the potential error you are describing. |
An example: Builders with new code hit the API (old code), the extra field will be returned, but the model from the builder doesn't have that field anymore. An error occurs. |
Well, that particular example won't happen since we deploy webs first. I get your idea, but I think it's not a problem, tho. |
We can remove the field itself after deploying this PR.