-
Notifications
You must be signed in to change notification settings - Fork 9
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
Chore: Faster devcontainer start #671
Conversation
The pre-commit "environments" (dependencies) now get installed during image build rather than devcontainer start. This shaves a minute off the start time on my machine.
It didn't sound like anyone (including GitHub Actions) was using Cypress in the devcontainer, so this skips that installation to speed up the devcontainer start time.
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.
Looks good to me. I do think we should have an alternative to running Cypress locally - maybe another container (though I think there are some challenges there).
I would like @machikoyasuda to weigh in especially from the Cypress POV.
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.
Good for me 👍
I confirmed that Cypress runs correctly with the existing Documentation instructions:
The one thing is that everyone now must be able to run Cypress locally - @angela-tran @thekaveman @afeld -- can y'all run the Cypress specs locally?
Cypress does support Docker Compose, though any time X11 is involved, I think it may be more trouble than it's worth 🙃 |
Oh goodness yes! I meant a container to run the CLI-based tests, not the UI! |
I can run it, though I get some failures. Might be worth a screen share to troubleshoot at some point, since I feel like I've heard others say the same.
Since we're already going to recommend people install Cypress locally and GitHub Actions runs it directly in the runner, not sure there's a ton of value. We can re-add later if needed. |
I realize I'm a bit late to this convo and that the branch has already been merged, which is totally fine. I just wanted to echo @thekaveman 's preference for having an alternative to running Cypress locally. I would use Cypress in the devcontainer to run tests with the CLI. The thing that didn't work from the devcontainer was launching the test runner UI. Hopefully we have some time in the future to look into defining a separate container. |
Also a late realization: this PR did not remove any of the node/npm installation steps that were only required because Cypress was installed in there too! Building the devcontainer from scratch still takes a while with all these steps... will post a quick follow-up. |
since Cypress was removed from the devcontainer install in #671 we don't need all the node/npm bits in there anymore
since Cypress was removed from the devcontainer install in #671 we don't need all the node/npm bits in there anymore
Punchline up front: This speeds up devcontainer start time by around five minutes on my machine.
The start time of a devcontainer was becoming a regular annoyance for me, enough so that I was finding myself working outside of it, which kinda defeats the purpose. I spent a bit of time to improve a couple of things that would make a big difference:
dev
image build time, which is infrequent/cachedWhile it doesn't complete all the tasks listed, I'm going to say this closes #495 as being "good enough."