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

Skip builds when project is not active #4991

Merged
merged 4 commits into from Dec 18, 2018
Merged

Skip builds when project is not active #4991

merged 4 commits into from Dec 18, 2018

Conversation

@humitos
Copy link
Member

@humitos humitos commented Dec 12, 2018

Allow to make the check extensible from output by defining an is_active method 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 None returned when project.skip was failing.

Needed by readthedocs/readthedocs-corporate#154
Closes #4257

humitos added 2 commits Dec 12, 2018
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.
@humitos humitos requested a review from Dec 12, 2018
@codecov
Copy link

@codecov codecov bot commented Dec 12, 2018

Codecov Report

Merging #4991 into master will increase coverage by 0.03%.
The diff coverage is 82.14%.

@@            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
Impacted Files Coverage Δ
readthedocs/restapi/views/integrations.py 91.95% <100%> (+0.18%) ⬆️
readthedocs/projects/querysets.py 81.48% <100%> (+0.89%) ⬆️
readthedocs/core/utils/__init__.py 80.41% <100%> (+5.67%) ⬆️
readthedocs/projects/views/private.py 79.63% <33.33%> (-0.42%) ⬇️
readthedocs/builds/views.py 90.74% <50%> (-5.26%) ⬇️

Copy link
Member

@ericholscher ericholscher left a 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.",
Copy link
Member

@ericholscher ericholscher Dec 12, 2018

Seems like this would happen a large number of times for a skipped project. Is this de-duped at all?

Copy link
Member Author

@humitos humitos Dec 12, 2018

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:

  1. project is mark as skip
  2. user click on Build version button
  3. user is redirected to Build List page
  4. this message appears

Next time the user visits any different page, this message won't be there anymore.

Copy link
Member

@ericholscher ericholscher Dec 12, 2018

ah ok. So this won't display when builds are triggered via GH and other options?

Copy link
Member Author

@humitos humitos Dec 12, 2018

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.
Copy link
Member

@ericholscher ericholscher Dec 12, 2018

not sure I follow this sentence

Copy link
Member Author

@humitos humitos Dec 12, 2018

😞

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 humitos force-pushed the humitos/projects/skip branch from fc948c4 to 7d0b78a Dec 12, 2018
@humitos
Copy link
Member Author

@humitos humitos commented Dec 12, 2018

Makes sense. Aren't we checking for project.skip in the build process as well: /readthedocs/projects/tasks.py@master#L443

The approach of this PR is to avoid projects that are marked to be skipped to trigger builds (besides using the is_active method to allow this check to be extensible from Corporate).

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 is_active in that line also and not only project.skip 👍

@humitos
Copy link
Member Author

@humitos humitos commented Dec 12, 2018

Mmm... the problem with that is that we don't have access to the database at that point and the checking from Corporate checks project.organization.disabled :/ So, not sure if we can modify it right now...

@humitos humitos force-pushed the humitos/projects/skip branch from 7d0b78a to a3993bd Dec 12, 2018
@readthedocs readthedocs deleted a comment from codecov bot Dec 12, 2018
@humitos
Copy link
Member Author

@humitos humitos commented Dec 17, 2018

I'm thinking that we can adapt the API response in that case so project.skip in corporate is project.skip or project.organization.disabled. I opened readthedocs/readthedocs-corporate#447 to track this in corporate.

I think we can merge this PR as is for now and come back after the discussion.

@humitos humitos requested a review from Dec 17, 2018
@humitos humitos merged commit 5d4da21 into master Dec 18, 2018
3 checks passed
@humitos humitos deleted the humitos/projects/skip branch Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants