Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve the devcontainer experience #3492

Merged
merged 24 commits into from
Nov 20, 2022
Merged

Improve the devcontainer experience #3492

merged 24 commits into from
Nov 20, 2022

Conversation

felipecrs
Copy link
Contributor

I decided to give a shot in the devcontainer today, and I realized some improvements could be made.

  • Do not require users to manually build the website before building container
  • Do not require users to manually build the base container (make local)
  • And some other minor things

@NickM-27
Copy link
Sponsor Collaborator

Personally, I would like to see the Dockerfiles stay separate. Much simpler to know which changes are for the main build vs just for the devcontainer. And then as other dockerfiles are being added like in #2548 the dev container will need to encapsulate all that as well getting more complicated.

@felipecrs
Copy link
Contributor Author

I'm just not sure how to achieve the same result by splitting the Dockerfiles.

@NickM-27
Copy link
Sponsor Collaborator

Yeah that makes sense, to avoid needing to make locally it'll have to be in the same file most likely

@felipecrs
Copy link
Contributor Author

Unless by:

Which is probably not going to be implemented any time soon.

blakeblackshear
blakeblackshear reviewed Jul 20, 2022
View reviewed changes
docker/Dockerfile Outdated Show resolved Hide resolved
@blakeblackshear
Copy link
Owner

Originally I had split the web build out because webpack would take forever on arm when doing builds in buildx for multiarch. It would also crash when building on the Pi because it would run out of memory. This may not be so bad because vite is so fast now.

@github-actions github-actions bot added the stale label Sep 27, 2022
@github-actions github-actions bot closed this Sep 30, 2022
@felipecrs felipecrs marked this pull request as draft November 14, 2022 18:08
@felipecrs felipecrs changed the base branch from release-0.11.0 to dev November 14, 2022 22:50
@netlify
Copy link

netlify bot commented Nov 14, 2022 •
edited
Loading

✅ Deploy Preview for frigate-docs canceled.

Name Link
🔨 Latest commit d2a2ba3
🔍 Latest deploy log https://app.netlify.com/sites/frigate-docs/deploys/63793dde0b75f90008bb4088

@felipecrs
Copy link
Contributor Author

This is ready to review again, demo:

Code_RKnN4heJiB.mp4

@felipecrs felipecrs marked this pull request as ready for review November 14, 2022 23:44
@NickM-27
Copy link
Sponsor Collaborator

I think this looks good to me upon trying it, running on macos all I need to do is comment out the devices in the docker-compose but this is already the case with todays container.

NateMeyer
NateMeyer reviewed Nov 15, 2022
View reviewed changes
Makefile Outdated Show resolved Hide resolved
blakeblackshear
blakeblackshear reviewed Nov 19, 2022
View reviewed changes
Makefile Outdated Show resolved Hide resolved
@felipecrs felipecrs changed the title Make it easier to run the devcontainer Improve the devcontainer experience Nov 19, 2022
@felipecrs
Copy link
Contributor Author

The testing repo (#4368) is causing some issues now. At first, it comes with a nodejs newer than the one we wanted and broke docker build, but now it's interfering with the installation of the vscode common utils. I'm still checking what I can do.

@blakeblackshear
Copy link
Owner

Did you see this suggestion? #4368 (comment)

@felipecrs
Copy link
Contributor Author

felipecrs commented Nov 19, 2022 •
edited
Loading

Did you see this suggestion? #4368 (comment)

I wasn't able to make such thing work like apt-get install -t nodesource nodejs. I don't think it can be used for such repos.

But I think I found a better solution, though. Check last commit.

felipecrs
felipecrs commented Nov 19, 2022
View reviewed changes
Dockerfile Show resolved Hide resolved
@felipecrs
Copy link
Contributor Author

Ok, should be ready to review again.

@blakeblackshear blakeblackshear merged commit 6582504 into blakeblackshear:dev Nov 20, 2022
@felipecrs felipecrs deleted the improve-devcontainer branch November 21, 2022 14:06
herostrat pushed a commit to herostrat/frigate that referenced this pull request Nov 24, 2022
@felipecrs @herostrat
Improve the devcontainer experience (blakeblackshear#3492)
c248ca0 
* Make it easier to run the devcontainer

* Some more improvements

* Tidy up few other things

* Better name stages

* Fix CI

* Setup everything with one click

* Allow to set IMAGE_OWNER

* Change IMAGE_OWNER to IMAGE_REPO

* Fix CI with IMAGE_REPO

* Fix nodejs installation

* Test devcontainer build as part of CI

* Build devcontainer in its own job

* Fix devcontainer cli installation

* Fix devcontainer build

* Fix devcontainer build in CI again

* Enable buildkit only

* Increase coverage of devcontainer test

* Fix devcontainer start in CI

* Ensure latest version of docker compose is used

* Fix install compose action

* Disable CI stuff which does not work until we fix them
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NateMeyer NateMeyer NateMeyer left review comments

@blakeblackshear blakeblackshear blakeblackshear approved these changes

Assignees
No one assigned
Labels
pinned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@felipecrs @NickM-27 @blakeblackshear @NateMeyer