-
Notifications
You must be signed in to change notification settings - Fork 211
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 a documentation page describing the different parts of the Openverse stack #3786
Conversation
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.
@zealshah29 this is a great start, thanks for the contributions. I do have some notes that I've elaborated on below.
-
You need to lint the file so that the CI check for linting can pass. You can do so using the
just lint
recipe. Linting will also run automatically from then on as the lint recipe will download pre-commit and set up the Git hooks for you. -
The newly added
stack.md
file needs to be added to the table of contents, which present is in the file https://github.com/WordPress/openverse/blob/3e57bfaff03d44b97e73ef5794ef0eda40b33749/documentation/general/index.md. -
Automation is not technically a part of the stack as it's more concerned with general repository management. I would drop it but I'll defer to other maintainers for their views.
-
The one-line descriptions for the stack layers can be found in the main
README.md
file, it'll be good to re-use those here. -
There are other components in our stack that are more general
purpose and shared between these layers. These include- cache (Redis)
- upstream and API database (PostgreSQL)
- anaytics (third-party, Plausible)
IMO these can, and should, be included in their own separate subsection.
- [Documentation](https://docs.openverse.org/api/index.html) | ||
- Language: Python | ||
- Tools and Framework: | ||
- [Django REST Framework](https://www.django-rest-framework.org/) |
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.
Also include Django above this.
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.
Shall I add it to line number 25?
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.
Yes, that would be great, thanks!
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.
@dhruvkb do you have an opinion about the docs links being pinned to a version? (5.0 in this case?) Not sure there's anything we can do about that 🤔
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.
Do I need to make any changes related to the link over here?
documentation/general/stack.md
Outdated
- Language: Python | ||
- Tools and Framework: | ||
- [Elasticsearch](https://www.elastic.co/guide/index.html) | ||
- [Docker](https://docs.docker.com/) |
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.
Docker is basically a dev + deployment mechanism and not a part of the stack's technologies so I recommend dropping that.
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.
Okay
documentation/general/stack.md
Outdated
- The public search engine at [openverse.org](https://openverse.org/), built with Vue and Nuxt. | ||
- [Code](https://github.com/WordPress/openverse/blob/main/frontend) | ||
- [Documentation](https://docs.openverse.org/frontend/index.html) | ||
- Language: JavaScript, HTML, CSS |
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.
HTML and CSS are kind-of universal to frontend dev so we can skip them. Also TypeScript is more appropriate here.
- Language: JavaScript, HTML, CSS | |
- Language: TypeScript / Node.js |
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.
Okay
documentation/general/stack.md
Outdated
- [Node.js](https://nodejs.org/docs/latest/api/) | ||
- [Tailwind CSS](https://v2.tailwindcss.com/docs) |
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.
Node.js is neither a tool nor a framework. It's a JS runtime, so it feels incorrect to list here.
As for TailwindCSS, it is a very small, not-noteworthy slice of the number of technologies we use in the frontend.
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.
Okay
documentation/general/stack.md
Outdated
|
||
|
||
|
||
|
||
|
||
|
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.
These blank lines should be removed as the lint job in CI will complain.
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.
Okay
Hi, can you share the documentation for step-by-step guidance on how to lint the file?
Shall I create a separate subsection for this? Or shall I add them under tools and framework for each stack? Also do we have a folder on the GitHub for the following files along with its documentation?
|
I search our docs and it turns out unfortunately, we do not have docs for that, which is a big miss on our end (and a potential opportunity for you to submit a PR for, once you've set it up for yourself). Linting locally requires a bunch of tooling set up like If you would like to not set them up, let us know and we'll lint the file for you.
Yes, a separate section with a name like "External services" would be good to group these services.
All of our external services are in the https://github.com/WordPress/openverse/tree/b4b0cc939dec200efc3bf6db55b402c67ed678c7/docker directory. Additionally, I missed some stacks like Documentation and some external services like Elasticsearch. Would be nice to include the documentation for them as well. Thanks again for this contribution and for being conducive to my feedback. |
Okay, I shall do the setup and try that on my own. In case I need help, I'll let you know.
Okay
Okay sure will go through it. So I'm planning to add them in the following manner: External Services:
Thank you for your guidance and support. |
Also, shall I remove Automations from stack.md? |
Hi so I tried installing just, but I am facing the following issue: Also, I checked the justfile and I read that it's not compatible with Windows OS. Since I am using windows OS (git bash), is there any way to do linting on my system? |
@zealshah29 Openverse development is only supported for Linux or macOS, largely because none of the core maintainers have Windows. For Windows, we recommend WSL to get a Linux-like environment which is much more conducive for development. However, in this case, we're happy to take over the PR and lint it ourselves. |
Hi, so I have updated the files as per the review. |
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.
Thank you @zealshah29! Just a couple notes about the Catalog and ingestion server, and then we can get the linting fixed :)
- [Documentation](https://docs.openverse.org/api/index.html) | ||
- Language: Python | ||
- Tools and Framework: | ||
- [Django REST Framework](https://www.django-rest-framework.org/) |
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.
@dhruvkb do you have an opinion about the docs links being pinned to a version? (5.0 in this case?) Not sure there's anything we can do about that 🤔
Yes sure, I shall make the necessary changes and share an update once it's done 👍🏻 |
Co-authored-by: Staci Mullins <[email protected]>
Added PostgreSQL's documentation link.
Hi, I have made the requested changes, could you please review it? |
Thanks for making those updates, @zealshah29! I think this is just about ready, but I've just noticed your PR is from your |
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.
On second thought, since this PR has been open for awhile and has a good bit of context, I'll approve as is! Please use a feature branch in the future, however.
Thanks again for your contribution!
Thank you! Sure I will take care of that from next time. |
Based on the low urgency of this PR, the following reviewers are being gently reminded to review this PR: @dhruvkb Excluding weekend1 days, this PR was ready for review 11 day(s) ago. PRs labelled with low urgency are expected to be reviewed within 5 weekday(s)2. @zealshah29, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
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 linting @stacimc. I agree with your reasoning to merge it and open separate issues later.
Fixes
Fixes #3201 by @AetherUnbound
Description
I have added a documentation page on the docs site that goes further in-depth about the various pieces of the stack, with references to the related documentation and code for each entity.
For each of the following sections of the stack listed, we have:
For example for the API, we have (respectively):
Testing Instructions
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin