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

Add basic support for robots.txt #82

Merged
merged 7 commits into from
Jun 24, 2019
Merged

Add basic support for robots.txt #82

merged 7 commits into from
Jun 24, 2019

Conversation

nathanielks
Copy link
Contributor

This is to prevent issues where services, such as Twitter, aren't able to generate a card because /robots.txt is missing from Tachyon's domain.

@nathanielks nathanielks requested a review from rmccue June 3, 2019 18:49
Copy link
Member

@rmccue rmccue left a comment

Choose a reason for hiding this comment

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

We might be better off handling this in API Gateway?

@nathanielks
Copy link
Contributor Author

@rmccue I had thought about that, but I assumed we'd want support for users who don't use API gateway so I included it in the function itself. What's your preferred method?

@nathanielks nathanielks requested a review from rmccue June 4, 2019 17:25
@rmccue
Copy link
Member

rmccue commented Jun 5, 2019

I assumed we'd want support for users who don't use API gateway so I included it in the function itself. What's your preferred method?

IMO, the "function" of Tachyon is to serve images, not to be a full server. The robots.txt stuff I think makes more sense in API Gateway, since that's the routing layer; we were just lazy and said "route everything to Tachyon".

We provide the API Gateway configuration, so we can PR it into there.

Maybe @joehoyle has other thoughts on this though?

@joehoyle
Copy link
Member

joehoyle commented Jun 5, 2019

I think it's fine being here for convenience, there's a fair bit more work when trying to use APIGateway, especially as that also will need to go through cloudformation.

As Japh mentioned, the \n looks wrong. Also, we don't need to include a robots.txt, the problem is it's a 500 now, a 404 would also be fine.

@nathanielks
Copy link
Contributor Author

Looks wrong, but it's not 😄

$ curl http://localhost:8080/robots.txt
User-agent: *
Allow: /%

(% in my terminal signifies end of the response)

@nathanielks
Copy link
Contributor Author

@Japh pingy ping

@nathanielks
Copy link
Contributor Author

@Japh @rmccue pingQ

@nathanielks
Copy link
Contributor Author

Hey folks! This is getting stale! @Japh @rmccue

@nathanielks
Copy link
Contributor Author

ᕕ( ᐛ )ᕗ Ping! @Japh

Copy link
Contributor

@Japh Japh left a comment

Choose a reason for hiding this comment

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

Naise!

@Japh Japh removed the request for review from rmccue June 24, 2019 15:55
@Japh Japh merged commit 23a3ab1 into master Jun 24, 2019
@Japh Japh deleted the add-robots.txt branch June 24, 2019 15:56
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.

4 participants