-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Proxito: actually cache robots.txt and sitemap.xml #10123
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
Conversation
Forgot to actually inherit from the CDNCacheControlMixin, so caching for these views isn't actually working on .com.
def _get_project(self): | ||
# Method used by the CDNCacheTagsMixin class. | ||
return self.request.unresolved_domain.project | ||
|
||
def _get_version(self): | ||
# Method used by the CDNCacheTagsMixin class. | ||
# This view isn't explicitly mapped to a version, | ||
# TODO: refactor how we set cache tags to avoid this. | ||
return None |
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.
I'm tracking how to improve this at #10124
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.
Makes sense to refactor this at some point 👍🏼
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.
Looks good. I think we only need to add a comment about why we are re-defining project_cache_tag
in these two views.
|
||
# Always cache this view, since it's the same for all users. | ||
cache_response = True | ||
project_cache_tag = "robots.txt" |
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.
It would be good to have a comment explaining why we are defining this here. I assume it's because we need one and only one tag per-project instead of per-version.
def _get_project(self): | ||
# Method used by the CDNCacheTagsMixin class. | ||
return self.request.unresolved_domain.project | ||
|
||
def _get_version(self): | ||
# Method used by the CDNCacheTagsMixin class. | ||
# This view isn't explicitly mapped to a version, | ||
# TODO: refactor how we set cache tags to avoid this. | ||
return None |
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.
Makes sense to refactor this at some point 👍🏼
Forgot to actually inherit from the CDNCacheControlMixin, so caching for these views isn't actually working on .com.