-
Notifications
You must be signed in to change notification settings - Fork 114
ADR: API feature parity to include static assets endpoint #98
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
Vercel Previews Deployed
|
| > UPDATE [Jan 2025] | ||
| > **We will now include this endpoint as part of our feature parity with the current Content API** |
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.
This is the only net new code. See existing same line in previous ADR here: https://github.com/hashicorp/web-unified-docs/blob/main/docs/decisions/adr-001-parity-with-existing-api.md?plain=1#L41
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.
This is not a requirement, but is there a reason that we changed our strategy that would be useful to add? Also fine as/is.
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.
@aaronvanderlip Me and Heat talked about it, and decided it wasn't a heavy lift and would allow us to remove that whole API from the mktg-content API. So it's still low priority, but now we are planning on moving that API over. (Before we had no plan on it.)
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.
Thinking out loud here: the current endpoint makes a request to Fathom to get analytics on the most visited pages. I wonder if that changes very much. If not, maybe we could make a static file. I don't love that this endpoint is reliant on a third-party service.
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.
@heatlikeheatwave I think it's generally fine as the static path endpoint is not a "critical path".
LeahMarieBush
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.
LGTM! Thanks for keeping the docs updated ❤️
RubenSandwich
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.
Thanks for keeping these up to date.
| > UPDATE [Jan 2025] | ||
| > **We will now include this endpoint as part of our feature parity with the current Content API** |
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.
This is not a requirement, but is there a reason that we changed our strategy that would be useful to add? Also fine as/is.
* ADR to include static assets endpoint * Add note about milestone planning and link to google doc * tidy --------- Co-authored-by: Ruben Nic <RubenSandwich@users.noreply.github.com>
Copied/pasted the feature parity ADR with the change of including the static assets API endpoint. Trying to keep our docs up to date as we go.
Feel free to suggest additional info and I will commit it to the branch.