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

Beacon generates robots.txt per site #700

Open
wants to merge 1 commit into
base: apb/sitemap
Choose a base branch
from
Open

Conversation

APB9785
Copy link
Contributor

@APB9785 APB9785 commented Dec 11, 2024

Resolves #219

Depends on #694, #687

@APB9785 APB9785 requested a review from leandrocp December 11, 2024 03:13
@APB9785 APB9785 self-assigned this Dec 11, 2024
Copy link
Contributor

@leandrocp leandrocp left a comment

Choose a reason for hiding this comment

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

Looking good. Just a suggestion to disallow the admin prefix.

@@ -0,0 +1,3 @@
# http://www.robotstxt.org
Copy link
Contributor

Choose a reason for hiding this comment

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

We could disallow the admin prefix here as well. See https://github.com/BeaconCMS/beacon_live_admin/blob/f8ff1621253c30c5f143655c11acf1116fa1c1e4/lib/beacon/live_admin/router.ex#L88

We need to use scoped_path to build the full path including the scope path. So if that attribute is present in the router we could include it as Disallow: <%= @admin_prefix %> otherwise just ignore the line since the admin might not even be included in the router.

And we'll need at least a simple test because that function might change and we need a failing test if that happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could disallow the admin prefix here as well

I don't think that will be included. This PR only uses the beacon_site macro for generation

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