Skip to content

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

Merged
merged 4 commits into from
Mar 13, 2023
Merged

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Mar 8, 2023

Forgot to actually inherit from the CDNCacheControlMixin, so caching for these views isn't actually working on .com.

Forgot to actually inherit from the CDNCacheControlMixin,
so caching for these views isn't actually working on .com.
@stsewd stsewd requested a review from a team as a code owner March 8, 2023 02:20
@stsewd stsewd requested a review from humitos March 8, 2023 02:20
Comment on lines +1050 to +1058
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
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'm tracking how to improve this at #10124

Copy link
Member

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 👍🏼

Copy link
Member

@humitos humitos left a 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"
Copy link
Member

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.

Comment on lines +1050 to +1058
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
Copy link
Member

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 👍🏼

@stsewd stsewd merged commit 73a05d2 into main Mar 13, 2023
@stsewd stsewd deleted the cache-robots-sitemap branch March 13, 2023 20:09
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.

2 participants