-
Notifications
You must be signed in to change notification settings - Fork 137
feat: serve markdown automatically based on Accept header
#1997
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
daf5be0 to
6c7c301
Compare
|
Preview for this PR was built for commit |
6c7c301 to
6690f27
Compare
|
Preview for this PR was built for commit |
6690f27 to
bc6a4e8
Compare
|
Preview for this PR was built for commit |
bc6a4e8 to
6da05f7
Compare
|
Preview for this PR was built for commit |
6da05f7 to
bae1097
Compare
|
Preview for this PR was built for commit |
|
Preview for this PR was built for commit |
bae1097 to
e9e37bf
Compare
|
Preview for this PR was built for commit |
e9e37bf to
6bac35b
Compare
|
Preview for this PR was built for commit |
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.
Pull Request Overview
This pull request adds automatic markdown content serving based on HTTP Accept headers. When a client requests text/plain or text/markdown content types, nginx serves the markdown version of pages directly instead of the HTML version.
- Implements nginx map directive to detect markdown-accepting clients
- Adds location-specific handling for different URL patterns (homepage, llms.txt files, and generic docs)
- Includes comprehensive CI tests to validate header-based content negotiation
- Fixes a typo in localhost IP address documentation
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| nginx.conf | Core nginx configuration implementing Accept header detection and conditional markdown serving |
| .github/workflows/test.yaml | Adds integration tests to verify proper Content-Type headers for various URL patterns |
| CONTRIBUTING.md | Fixes typo in localhost IP address (127.0.01 → 127.0.0.1) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Preview for this PR was built for commit |
|
Great job guys! Pls let me know once it's deployed, will test it |
|
One thing I just realized, this is only working for the apify-docs stuff, not for CLI, SDKs, and clients. I'd first get this out before I try to make it work there. |
| - name: Start Docusaurus server | ||
| run: | | ||
| nohup npx docusaurus serve --port 3000 --no-open & |
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.
Instead of this nohup and sleep goulash, I suggest using background-action like I do here https://github.com/janbuchar/website/blob/cac7908f1834a79788264f1ca170fc70717d29b0/.github/workflows/deploy.yaml#L47-L52
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 guess we need apt install wait? Or how come it works for you in the other repo? :D 0e1d0e2
Error: [Error: ENOENT: no such file or directory, lstat '/home/runner/work/apify-docs/apify-docs/wait'] {
edit: reverting for now, can't find anything about why this fails
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.
Good call with the tests, I never would've believed this without them
|
Preview for this PR was built for commit |
|
Preview for this PR was built for commit |
24376b7 to
7b4ff27
Compare
|
Preview for this PR was built for commit |
7b4ff27 to
e5a4bfa
Compare
|
Preview for this PR was built for commit |
lukasmrtvy
left a comment
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 would suggest using containers (https://docs.github.com/en/actions/how-tos/write-workflows/choose-where-workflows-run/run-jobs-in-a-container) directly inside the workflow, otherwise apt might be slow and unstable.
Alters the nginx config so it checks the
Acceptheader, if it containstext/plainortext/markdown, we serve the markdown version directly. Also added tests for this behavior to the existing PR check.The previews are likely using the config from master, so there it doesn't work yet (it should after we merge this).
Since the test workflow now effectively verifies the nginx config is valid, I guess we can drop the
nginx.conf-test.yamlworkflow.