-
Notifications
You must be signed in to change notification settings - Fork 18
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
Update the CONTRIBUTING guide #195
Update the CONTRIBUTING guide #195
Conversation
CONTRIBUTING.md
Outdated
[scrubbed](https://github.com/LibriVox/librivox-ansible/blob/master/dbscrub.py) | ||
of all personal information. | ||
The [librivox-ansible] playbooks can be used to set up a local development | ||
server. It comes with a [database snapshot] that has been |
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.
s/It comes/They come/
I know you didn't actually write this, but if we're updating this, could you fix this small nit as well please?
CONTRIBUTING.md
Outdated
server. It comes with a [database snapshot] that has been | ||
[scrubbed][database-scrub-script] of all personal information. | ||
|
||
Once you've got all that running nicely, and your hosts file set up, you can |
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 reads weird, as if we've skipped all the important steps of actually getting it all running nicely. Maybe move this below Getting Started, or add a small paragraph on how to actually run the playbooks?
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.
My preference would be to leave the exact details to the librivox-ansible README, that way it can be changed and updated without having to come here and change anything.
On a slightly-related topic, I see that the Github Actions set up librivox inside LXC containers. Do you do the same locally when you're developing? If so, it might be worth sharing how to get set up with it here so that others can do the same.
I have everything set up inside Docker locally, though it takes some messing around to get it running right the first time, so I don't think that it's all that suitable to refer to: https://github.com/garethsime/librivox-localdev-with-docker (After the initial setup, I can just run ./start.sh
and I instantly get a fresh container, but yeah, takes some work to get to that point.)
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 have a stale development VM locally somewhere, but I mostly (ab)use the staging server.
In production and staging, the requirement driving the use of LXC is the ability to run both production and staging instances on the same server. Our server at IA is already a VM, so I went with containers for the prod and staging instances. Nested VMs are possible, but are more finicky than LXC, so there was no reason to bother when LXC does the job.
Parenthesis: There are also historical reasons - docker (and LXC's younger brother LXD) wasn't around when we were first starting out with IA hosting (I actually looked it up - we moved servers in 2012 [1] and Docker was first released in 2013 [2]). Also, our initial setup was more complex, with two VMs from IA, and an attempt at active/passive failover, necessitating the ability to start the production instance from shared storage on the second server. This ended up going away when we upgraded to the latest Ubuntu LTS in 2022, as the complexity was causing more problems than it was solving, and the messaging from the admins was that I shouldn't be chasing five nines uptime.
GitHub Actions uses LXC because it can't run the Ansible playbooks natively (makes me miss Zuul from my day job even more), so we need an Ansible executor (the VM that GitHub Actions run on), and a separate target. LXC was just the easiest way to get that.
I'm weary of specifying a specific technology to do LV dev locally, so I'd like your opinion. I've had previous contributors write a guide for VirtualBox, you're using Docker, and I have a qemu+kvm VM somewhere. All of these work, and neither is better than the others, and are frankly out of scope of LV itself. What's your take on being prescriptive, potentially removing the learning curve, but overstepping scope and removing choice - vs not specifying anything except the bare requirements (Jammy, users, networking) and allowing folks to chose, but adding a potential barrier to entry?
[1] https://forum.librivox.org/viewtopic.php?t=43196
[2] https://en.wikipedia.org/wiki/Docker_(software)
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 the thorough explanation, and it's cool to hear about both how the webapp is deployed and the history of the thing 🙂 It certainly gave me a lot to think about today.
I'm weary of specifying a specific technology to do LV dev locally, so I'd like your opinion.
My motivation for it was that I found it quite hard to get set up the first time. (It took me a few attempts as I initially tried before the Ubuntu 2022.04 upgrade and a lot of the dependencies were hard to get by that point. After the upgrade, things went much smoother.) This was also my first brush with Ansible, so I had to go and learn a bit of that first.
One thing I didn't notice before was that you'd already updated the librivox-ansible README a few months ago, and I think it makes it clearer about what the setup needs to be and what changes you have to make to the inventory.yml
. I would expect people who are familiar with VMs/containers to come right with those instructions, but if someone hasn't come across them before, then they'll probably have to go learn that first. (Not a bad thing, maybe? Depends on your skillset/interests)
I've had previous contributors write a guide for VirtualBox, you're using Docker, and I have a qemu+kvm VM somewhere.
Yeah, that's a good point, the technology does evolve over time and maybe Librivox doesn't want to get too bogged down in any one thing. I balked at the idea of having to use VirtualBox now, but 2014 me would've been so into that.
What's your take on being prescriptive, potentially removing the learning curve, but overstepping scope and removing choice - vs not specifying anything except the bare requirements (Jammy, users, networking) and allowing folks to chose, but adding a potential barrier to entry?
I don't think we have to choose between the two, we can say "This is the base requirement, and here's a suggestion for how you meet that", which leaves it open for people to do their own thing while giving those who just care about the code an easy path forward. A nice in-the-middle approach might be to keep librivox-ansible as it is now with the bare minimum requirements, but link to a few "community" or "unofficial" guides in other repos, so people can pick-and-choose a bit with the understanding that Librivox itself isn't committed to supporting any one tool.
My gut feel is that making the project easier to approach will help build a wider and more skillset-diverse developer base, which might be good for long-term stability of the project. (It might make more work for you reviewing and testing changes though haha.)
Take all this with a grain of salt though, this is my first real contribution to open source, so I have no idea what a good strategy for this kind of project is. I'm also not a core member of the project, so I have very little skin in the game. If this was my day-job, then I'd be 100% in on being prescriptive because you're kind of committed to making sure that all of your coworkers can contribute, so standard tooling makes debugging setup issues easier and it's easier for new joiners to get started.
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 don't think we have to choose between the two, we can say "This is the base requirement, and here's a suggestion for how you meet that", which leaves it open for people to do their own thing while giving those who just care about the code an easy path forward. A nice in-the-middle approach might be to keep librivox-ansible as it is now with the bare minimum requirements, but link to a few "community" or "unofficial" guides in other repos, so people can pick-and-choose a bit with the understanding that Librivox itself isn't committed to supporting any one tool.
I like this a lot. We can probably even keep those in-repo, under a community
or extra
directory or something.
My gut feel is that making the project easier to approach will help build a wider and more skillset-diverse developer base, which might be good for long-term stability of the project. (It might make more work for you reviewing and testing changes though haha.)
Well, if we make it clear that the community/unofficial/extra guides are best effort, and not officially maintained, I don't even have to do that :D The onus would be on the original author to keep it up to date and tested. Perhaps with the caveat that I can remove guides that are obviously out of date and broken.
Take all this with a grain of salt though, this is my first real contribution to open source, so I have no idea what a good strategy for this kind of project is. I'm also not a core member of the project, so I have very little skin in the game. If this was my day-job, then I'd be 100% in on being prescriptive because you're kind of committed to making sure that all of your coworkers can contribute, so standard tooling makes debugging setup issues easier and it's easier for new joiners to get started.
You're doing fine :) My day job is open source, and while I'm no guru, I've seen my fair share of new contributors show up to projects, and the maturity and thoughtfulness with which you approach things is far beyond what I've seen in most newbies.
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.
Hi, sorry it's taken me a wee while to get back to this 🙂 Thank you for the kind words!
The onus would be on the original author to keep it up to date and tested.
Totally, I just mean that easier contribution should (hopefully) mean more contributors, which means more Librivox PRs for you to review :P
I will work on tidying up my docker-based guide separately, and if I get that into a nice enough state, then we can talk about maybe including it as a community guide.
For the time being, I've tweaked the wording on this paragraph to try and make it read better, which was the original comment, so let me know if the new version makes more sense! I think it would be worth getting this merged even if it's not 100% perfect because it's probably better than what we've got now
CONTRIBUTING.md
Outdated
|
||
# Writing tests | ||
|
||
We don't have many tests yet, but we're working on it! Providing tests with 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.
Not true anymore! We have tests now! ;)
CONTRIBUTING.md
Outdated
|
||
For testing, we use [phpunit] with [ci-phpunit-test]. You can find our tests | ||
under [`application/tests`](./application/tests), and there's a README there | ||
that describes how to get started using [composer]. |
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.
Now that we have composer.phar in-tree, it might be good to just give the command here, which IIUC is just:
$ php composer.phar install
Thanks for taking a look! I'm out for a few days, but I can make the updates when I get home |
7255cdf
to
9245899
Compare
It took me a fair bit of googling and playing around to get a grip on how librivox-catalog fits together, so I thought I'd start writing some of it down for the next person. I haven't included any details on the structure of the code itself, but I've included some links out to the CodeIgniter docs, which should go a little way to helping people get started.
9245899
to
b9cc5af
Compare
If you (or anyone else, really) ends up coming back to this, I'd like to get rid of the first person singular ("I") in the couple of spots where it's used. Either using the passive voice, or first person plural ("we"). |
No problem, I've raised PR #216 to remove it 🙂 EDIT: Linked the wrong PR number |
At the request of garethsime, I here include the steps I have taken on a Mac running Ubuntu 22.04 under Parallels Desktop to install the Librivox Catalog development environment. Install ansiblesudo apt update Install ansible playbookcd ~
Run the following command: Install WordPressStart Firefox. Enter URL http://librivox.org Click "Advanced" button, then Accept the "Risk and Continue". I set the following values. There appear to be no wrong answers here! Click "Install Wordpress". Verify installationIn Firefox, go to http://librivox.org/search A comment about this processIn practice, it took me hours to get to the point where I realised the process could be (and should be) this simple. I have, however, just installed the Librivox development environment on a second Mac running Ubuntu 22.04 under Parallels Desktop and verified that the above instructions worked for me as written. |
Thanks for the write-up, @peterjdann! 🙂 It's cool to see that the playbooks work well regardless of how the Ansible host is set up (Parallels, Docker, LXC, etc.), so hats off to @notartom for the love that's gone into them
Yeah, I found much the same the first time I set it up locally. In hindsight, it's really not that hard, but it felt tricky at the time haha
|
It took me a fair bit of googling and playing around to get a grip on how librivox-catalog fits together, so I thought I'd start writing some of it down for the next person.
I haven't included any details on the structure of the code itself, but I've included some links out to the CodeIgniter docs, which should go a little way to helping people get started.