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

Remove localmendia and search parameters #4895

Merged

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Nov 12, 2018

These aren't used anywhere, as we always depend on the config (or project) for this decisions.

I'm also removing the HTML_ONLY_PROJECTS setting, I guess that was acting as a feature flag a long time ago when users couldn't disable epub and pdf by themselves?

@codecov
Copy link

codecov bot commented Nov 12, 2018

Codecov Report

Merging #4895 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4895      +/-   ##
==========================================
+ Coverage    76.6%   76.64%   +0.04%     
==========================================
  Files         158      158              
  Lines       10043    10041       -2     
  Branches     1268     1269       +1     
==========================================
+ Hits         7693     7696       +3     
+ Misses       2008     2005       -3     
+ Partials      342      340       -2
Impacted Files Coverage Δ
readthedocs/projects/tasks.py 71.45% <100%> (+0.89%) ⬆️

@stsewd
Copy link
Member Author

stsewd commented Nov 12, 2018

https://codecov.io/gh/rtfd/readthedocs.org/pull/4895/diff seeing that, I need to add tests for the mkdocs case p:

@@ -62,8 +62,6 @@

log = logging.getLogger(__name__)

HTML_ONLY = getattr(settings, 'HTML_ONLY_PROJECTS', ())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that we have 3 projects here, with a comment banned, I think we can move those to use the skip field on the db.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skip will skip the build entirely, sounds like we want to disable non-html builds instead. i don't think we have a way to opt users into disabling pdf/epub from our side other than this setting right now. it could just be a feature flag or something though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you check the settings from the ops repo, there is this comment banned, and if these projects are abusive projects, probably the best to do is ban them (skip)? otherwise, the builders would time out or kill the process when building pdf/epub anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like of the 3 projects, 1 is deleted, and the others haven't been built for 4 months and 1 year, so I think we're probably safe to remove it, and deal with it if it hit issues again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me

@stsewd stsewd closed this Nov 12, 2018
@stsewd stsewd reopened this Nov 12, 2018
@stsewd
Copy link
Member Author

stsewd commented Nov 12, 2018

note: I just close/reopen this to trigger travis

@stsewd stsewd requested a review from a team November 13, 2018 04:49
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to make sense to me I think, though I think this will require complimentary changes on our commercial code as well.

The HTML_ONLY wasn't just used as a feature flag, but it also was used for abusive projects. It was mostly a hack, but we do need to preserve the functionality I believe.

@@ -62,8 +62,6 @@

log = logging.getLogger(__name__)

HTML_ONLY = getattr(settings, 'HTML_ONLY_PROJECTS', ())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skip will skip the build entirely, sounds like we want to disable non-html builds instead. i don't think we have a way to opt users into disabling pdf/epub from our side other than this setting right now. it could just be a feature flag or something though.

@ericholscher
Copy link
Member

I'm curious if the projects in prod are still active. We can probably just ping each 4 of them and ask them to configure formats, and be done with it.

@stsewd
Copy link
Member Author

stsewd commented Nov 16, 2018

This seems to make sense to me I think, though I think this will require complimentary changes on our commercial code as well

I always wanted an excuse to touch that code 😄

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we should ship this.

@@ -62,8 +62,6 @@

log = logging.getLogger(__name__)

HTML_ONLY = getattr(settings, 'HTML_ONLY_PROJECTS', ())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like of the 3 projects, 1 is deleted, and the others haven't been built for 4 months and 1 year, so I think we're probably safe to remove it, and deal with it if it hit issues again.

@agjohnson agjohnson added this to the 2.9 milestone Dec 8, 2018
@stsewd stsewd mentioned this pull request Dec 13, 2018
@stsewd stsewd requested a review from agjohnson December 17, 2018 17:56
@ericholscher ericholscher merged commit 562a6a1 into readthedocs:master Dec 26, 2018
@stsewd stsewd deleted the remove-search-and-localmedia-param branch December 26, 2018 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants