search

metal3-io / metal3-docs

Architecture documentation that describes the components being built under Metal³.
http://metal3.io
Apache License 2.0
263 stars 111 forks source link

✨ Added placeholders that resolve to the latest version tag of a release #381

Closed peppi-lotta closed 4 months ago

peppi-lotta commented 4 months ago

Added placeholders that will be resolved in the latest version tag of the given project. Version numbers are resolved after the page is rendered with JavaScript.

Known issue: User will see placeholders if JavaScript is disabled in browser.

metal3-io-bot commented 4 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign kashifest for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/metal3-io/metal3-docs/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
mquhuy commented 4 months ago

The implementation looks good, thanks @peppi-lotta. I have a question with this design choice though: If I understand correctly, the versions will have to be resolved with github api every time someone reads the book, i.e. not just at build phase, right? It means if someone refreshes the browser multiple times in a row, they might go over the API limit. IMO it would be much better to do it just once at render phase, and let the final html have the versions already rendered. I saw that mdbook has some Custom backend syntax, have you checked if we could use that instead?

peppi-lotta commented 4 months ago

The implementation looks good, thanks @peppi-lotta. I have a question with this design choice though: If I understand correctly, the versions will have to be resolved with github api every time someone reads the book, i.e. not just at build phase, right? It means if someone refreshes the browser multiple times in a row, they might go over the API limit. IMO it would be much better to do it just once at render phase, and let the final html have the versions already rendered. I saw that mdbook has some Custom backend syntax, have you checked if we could use that instead?

To my understanding renderers are used when the book is build. The book is build and then deployed to Netlify. This means that after every release someone would have to rebuild and deploy the book because once in Netlify the content is already static. This is why I used the placeholders to work around the static content to fetched and updated the placehoplders when the client makes the request to see the book.

The comment about the api rate limits is very valid. I'm thinking I could cache the version values as session variables or something so that an api requests would be made for each unique url and if the same url is in another placeholder then the cached value would be used instead of fetching.

mquhuy commented 4 months ago

The implementation looks good, thanks @peppi-lotta. I have a question with this design choice though: If I understand correctly, the versions will have to be resolved with github api every time someone reads the book, i.e. not just at build phase, right? It means if someone refreshes the browser multiple times in a row, they might go over the API limit. IMO it would be much better to do it just once at render phase, and let the final html have the versions already rendered. I saw that mdbook has some Custom backend syntax, have you checked if we could use that instead?

To my understanding renderers are used when the book is build. The book is build and then deployed to Netlify. This means that after every release someone would have to rebuild and deploy the book because once in Netlify the content is already static. This is why I used the placeholders to work around the static content to fetched and updated the placehoplders when the client makes the request to see the book.

The comment about the api rate limits is very valid. I'm thinking I could cache the version values as session variables or something so that an api requests would be made for each unique url and if the same url is in another placeholder then the cached value would be used instead of fetching.

Thanks for the explanation. I like the idea of having placeholders, but I think we shouldn't need to update it all the time. I don't like the idea of using DOM manipulation to render this, also because it goes against how mdbook is intended to be used. Mdbook is supposed to be fast and lightweight, due to the static html nature. By doing this (and adding client-side JS in general), we add several ms to almost every load.

Caching is a way to solve the API limit, but I'm still not sure if it can be done on the client side. Anw my JS knowledge is limited so please feel free to educate me with how you intend to implement it.

IMO it's sufficient that we re-render the Netlify app once a week or so to update the versions. This could be very well-automated, for e.g. with Netlify deploy API.

peppi-lotta commented 4 months ago

Thank you for comments and reviews! I will close this PR and make another for an approach where placeholders are resolved at build.