Closed mihaitodor closed 1 year ago
Hi @mihaitodor! That sounds totally reasonable, and looking at Keda's example, that doesn't look too daunting from the maintenance perspective either. I'm fine with this.
(I'm not a VS Code user, but IIRC @CaselIT is, so he can probably provide more feedback here.)
I honestly never even looked at the mintest.sh script.. Looking at it I'm not really sure it's that useful as is now. Maybe we should update (or even remove it?)
Regarding vscode I've never used the devcontainer feature, but as @vytas7 said it doesn't look to hard to support.
But first I would definitely think about what we want to do with mintest.sh
Let's maybe create a separate issue if you believe mintest.sh
needs attention, and this one could track the devcontainer feature?
I think it would impact the devcontainer, since it installs items that are needed by that script
Hm... Do you have any specific issue with that mintest.sh
in mind? (It probably ain't pretty, but it does the job, and if gets automated in a devcontainer, probably the details don't matter too much.)
Considering that the tests that run in the pr anyway, I think a single tox -e pep8 py
is enough in the contributing doc.
It seems very overkill what we have now, especially py3.5
Hm, good point. I think that's the way the majority of contributors go anyway, they just throw stuff at the CI and see if it sticks :slightly_smiling_face:
However, restricted checks according to your proposal wouldn't check full code coverage :thinking: Which is at least something that I check before opening a PR.
Sure, but I don't really think that's needed to open a pr. I mean it may make contributing difficult if just to submit a pr a lot of setup is required.
I personally test a single python version, then use the ci to check if a particular version has issues. It's why we have it, so we don't have to do all checks ourselves :)
maybe mintest may stay as is, we may just simplify the contributing guidelines
Yeah, maybe we could say that you can test coverage etc with mintest.sh
(and by the way, if you happen to use VS Code, it's easy with devcontainers), but if you find that daunting, don't worry, just submit a PR, and CI will find issues.
However, one drawback of this approach is that GH Actions don't run automatically for new contributors...
Regardless, I agree that we should do everything possible to lower the barrier for new contributors.
However, one drawback of this approach is that GH Actions don't run automatically for new contributors...
they do run on their fork. They don't run on our repo, and that's ok :)
OK, so just to summarise:
mintest.sh
the way it currently is, so no new issue needed for that@mihaitodor are you still interested in creating a devcontainer for Falcon?
We have otherwise cleaned up those tests scripts, just tox
(without any special parameters) is now enough to invoke the standard set of environments equivalent to the previous mintest.sh
.
@vytas7 Ugh, I really should've followed up on this... Thanks for the reminder!
I took a quick stab at it in #2132. Would be interesting to hear what you guys think of the ergonomics. It does add some overhead to opening the project the first time, but, once that's done, the container will be stopped when VS Code is closed, so it can be resumed when VS Code is opened again. I believe the tests run a bit slower inside the container, since the virtual machine that Docker spins up adds a bit of overhead. I'd be curious to hear what is the difference on your machine. I ran tox
successfully in the container and then I ran time tox
and it took 5m38s on a Macbook from 2013 with 4 GB RAM and 6 CPUs allocated for Docker. I didn't bother to see what I need to install locally to run it outside of Docker :)
I can imagine scenarios in which people who aren't familiar with Docker will get tripped up, like the fact that any terminal they open inside VS Code when running in the Dev Container will not have most of the tools they might be using locally (stuff like RipGrep or The Silver Searcher or various other network utils, perf analysis tools etc). They might also get tripped up if they launch some Falcon test app inside the container and then try to access it from their browser, only to notice that it doesn't work, because the port wasn't exposed on the host... Lots of stuff to consider. However, it probably lowers the entry bar for some people who have a hard time getting the tests to run locally. Worst case, nobody will use it :)
@mihaitodor no worries, thanks for getting back! :slightly_smiling_face: I'm not using VS Code myself, @CaselIT what do you think? Could you test the proposed dev container?
I'll take a look. I've never used it myself, but i have docker and vscode so it should be testable :)
Regarding documenting it, maybe we could mention it in the contribution docs? Do you have suggestions @vytas7
Adding a couple of sentences to CONTRIBUTING.md
just after the chapter about running tox
sounds good.
I struggled a bit to get
tools/mintest.sh
to run and since I'm not a very experienced Python developer, setting up the prerequisites on OSX proved to be a challenge. Having multiple versions of Python in parallel seems a bit error-prone to me. I guess usingpyenv
from the start is the way to go, but wasn't sure based on the instructions from here. On the other hand, I recently played around with a project called Keda and I found their devcontainer setup quite easy to use, assuming one is willing to run Docker. Basically, VS Code starts a headless server in the container automatically and then the UI communicates with it seamlessly.For Falcon, I found it easier to install all the dependencies in a Docker container and run everything there without worrying about messing up Python on my local laptop:
I can easily wrap the above in a devcontainer if you'd like.