Skip builds when project is not active #4991
Conversation
Allow to make the check extensible from output by defining an `is_active` method in the QuerySet. Also, this commit fixes different places where the `None` returned when `project.skip` was failing.
Codecov Report
@@ Coverage Diff @@
## master #4991 +/- ##
=========================================
+ Coverage 76.76% 76.8% +0.03%
=========================================
Files 158 158
Lines 9953 9970 +17
Branches 1245 1249 +4
=========================================
+ Hits 7640 7657 +17
Misses 1980 1980
Partials 333 333
|
There was a problem hiding this comment.
Makes sense. Aren't we checking for project.skip in the build process as well: https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/projects/tasks.py#L443
| messages.add_message( | ||
| request, | ||
| messages.WARNING, | ||
| "This project is currently disabled and can't trigger new builds.", |
There was a problem hiding this comment.
Seems like this would happen a large number of times for a skipped project. Is this de-duped at all?
There was a problem hiding this comment.
This only happens when the build is manually triggered and we are using regular Django message system which adds the message into the request and the message is consumed immediately in the next response.
So, the flow is:
- project is mark as skip
- user click on
Build versionbutton - user is redirected to Build List page
- this message appears
Next time the user visits any different page, this message won't be there anymore.
There was a problem hiding this comment.
ah ok. So this won't display when builds are triggered via GH and other options?
There was a problem hiding this comment.
No. When triggered via webhook I'm returning a 406 status code and a detailed message with the explanation of the situation.
| Check if the project is active. | ||
|
|
||
| The check consists on, | ||
| * the Project shouldn't be marked as skipped. |
There was a problem hiding this comment.
not sure I follow this sentence
There was a problem hiding this comment.
😞
I just want to say that it checks for project.skip. That's why the project must not has to be marked as skipped... :)
humitos
force-pushed
the
humitos/projects/skip
branch
from
fc948c4 to
7d0b78a
Compare
The approach of this PR is to avoid projects that are marked to be skipped to trigger builds (besides using the BUT, if the project has already triggered hundred or thousands of builds, the only way to stop them from building is by doing this check from inside the Celery task. So, I think it's fair to use |
|
Mmm... the problem with that is that we don't have access to the database at that point and the checking from Corporate checks |
humitos
force-pushed
the
humitos/projects/skip
branch
from
7d0b78a to
a3993bd
Compare
|
I'm thinking that we can adapt the API response in that case so I think we can merge this PR as is for now and come back after the discussion. |
Allow to make the check extensible from output by defining an
is_activemethod in the QuerySet. Besides, returns a proper HTTP status code and message when the project is disabled and the build is triggered via a webhook.Also, this commit fixes different places where the
Nonereturned whenproject.skipwas failing.Needed by https://github.com/rtfd/readthedocs-corporate/issues/154
Closes #4257