-
Notifications
You must be signed in to change notification settings - Fork 212
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
Rough draft of contributor help script #4293
Conversation
Full-stack documentation: https://docs.openverse.org/_preview/4293 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. Changed files 🔄: |
I'd love to review this PR in depth soon, but I do want to quickly chime in to voice my support for keeping the docs in the docs, and for the script to print links to the docs site. For these reasons:
|
I agree with you, Dhruv. Do you think we should have the instructions say something like:
That would change the docs from "install these dependencies" to "run this script and it will tell you what to do next". It feels like there's a bit of a structural change warranted by the introduction of this script, mainly because I'm not entirely confident when it should be brought up in the documentation. |
We can suggest the script at the very top of the general setup page, in a "tip"-style admonition. This lets novice contributors, who will primarily be interested in a script like this to find it immediately instead of having to read through any docs first. I'm not a fan of the curl https://raw.githubusercontent.com/WordPress/openverse/main/get_started.sh | bash Then the entire rest of the page can be left as-is as reading material for slightly advanced contributors who would prefer to set things up manually with more control over their systems. Any missing deps can be listed by the script as links to the very specific sections in the file for those deps. It would love it if we could go one step further and just install the missing deps for the users too (but that can make the script extremely complicated and, since we'll need Oh and I think the script name Additionally I feel like the script should ask the contributor what part of the stack they would like to work on because some setup can be avoided, and some deps may differ, if they decide to only work on a smaller part of the stack like the docs or the frontend (which do not need Docker) or the API or ingestion server which do not need PDM to be present locally. Also about linting, I think we can keep the entire thing as optional and let the CI check it for them. Linting makes a lot of dependencies like |
Sounds good on changing the script to link to the docs, it simplifies things quite a bit!
I wondered about this approach as well, but also am not a fan of it 🤔 Git is typically easy to install anywhere we support development 🤔. It's available by default on many Linux distros and macOS (as far as I understand it).
FWIW, just to clarify, I'm firmly against this for a lot of reasons, even if we could work around the need for If anything, we could simplify things by ditching Installing things ourselves presents a huge list of problems, which I wrote about in the issue. I don't think it's something we should get in the mindset of trying to manage in the project unless we managed to move the entire development environment into Docker and made Docker the only dependency. That would be awesome, but would require making sure docker socket pass through was intuitive and easy for everyone to get working regardless of context. Doing that would avoid all of this, Docker is trivially easy to install these days pretty much everywhere.
That would be great, but maybe a fast follow? I agree it's a good feature, but more significant, and there are already some big questions that need answering in this basic version. What do you think about this compared to making a development environment in a Docker image and Docker being the only requirement? We'd still need to write some kind of initialisation script to configure git inside the container, I think? Or could require git and docker, and everything else happens inside an openverse-dev-env Docker container? I played around a bit with nix because it's another option for creating a zero-effort development environment... but only if nix is already available, and that's a question in itself 😅
The issue here is that then contributors are not running linters, and we repeatedly have to ask them to do so in PRs, or run it ourselves. This is a massive increase in the time and friction it takes for us to review PRs. I've talked about this in the discussion on this PR, #3889, but I think running the linters is a reasonable baseline expectation that you must be able to do to contribute. Same with writing unit tests when a change would require them. We need to have some baseline expectation of being able to run development tools and follow the quick start guide. There are essentially no contributions that are less complex than that, and while it's nice to be able to accept easy one-off contributions, I question the value of them in and of themselves if we have to spend more time requesting changes or running linters on a PR ourselves than it would have taken to just implement the issue. As discussed in that PR, I don't think Openverse has the context, time, or other resources to be a significant aspect of someone's learning the basic skills of software development in Python or JavaScript. It would be great if we did, but we're very small team working on an ambitious project, and already are bogged down for time and energy to tackle our most urgent and pressing needs. |
I would be all in on the devcontainers workflow where 100% of the development happens in a container with everything pre-provisioned but that has not worked out so reliably. I remember this issue with executables github.com/docker/for-mac/issues/5029. For now, I think a simple script that only checks for all our dependencies (PDM, pnpm, Docker... everything) and reports a list of missing ones with docs links will be good enough. If it can be run with What I mean to say this is a good start and I'd be totally open to merging this PR as-is albeit after a proper review with further enhancements as needed based on contributor feedback. |
I'm going to close this for now, because I think #4343 is a better approach that fixes all the problems we wanted to solve with this script, without introducing a bunch of complex caveats about how to install certain dependencies on systems where it's a pain to do so for one reason or another. If we can just entirely obviate the need to think about any of this for a normal contributor, and I believe we can by using just git, bash, and docker, then I'd prefer that than trying to sort out the complexity of instructing contributors to use this script (or not) and the documentation to support it. #4343 would also be something that all of us could use all the time when working on Openverse, and so we'd be confident that it would continue to work and remain true to the needs of the project. A get-started script like the one in this PR would essentially never be used by us directly, and probably go out of sync with the project's needs. |
Fixes
Fixes #4137 by @AetherUnbound
Description
Adds a new script
get-started.sh
for contributors to run. The script checks for dependencies on their system and gives suggestions for how to install them.I've gone with a basic approach here, and we can expand or change references however we see fit. One thing I've noticed is that the installation suggestions naturally overlap with the documentation we have in the getting started page. Is there a reasonable way to merge these? Should we go "all in" on the script? Or should the script refer to the documentation pages instead of putting the suggestions in-line? The latter is probably the clearest and easiest way to remove the duplication. What do y'all think?
Testing Instructions
I've added a temporary Dockerfile and just recipe to make testing the different scenarios as easy as possible.
Run
just test-get-started <target>
with each of the targets defined inDockerfile.get-started-test
:I'll remove the Dockerfile and the just recipe before merging.
Output when there's nothing missing
Output when everything is missing
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin