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.

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

Add docker-native module for native docker support in NixOS #82

Merged
merged 3 commits into from
Apr 21, 2022

Conversation

ajaxbits
Copy link
Contributor

Addresses #59 by overlaying legacy iptables on the docker package.

Addresses [nix-community#59](nix-community#59 (comment))
by overlaying legacy iptables on the docker package.
@grantbevis
Copy link

This looks good to me code wise @nzbr but I haven't the means to test this currently - Are you able to test?

@grantbevis grantbevis self-assigned this Apr 21, 2022
@grantbevis grantbevis added the enhancement New feature or request label Apr 21, 2022
@grantbevis
Copy link

Fixes #59

@nzbr
Copy link
Member

nzbr commented Apr 21, 2022

It works (just tested it). I have a patch to rename the old docker option to docker-desktop. GitHub does not allow attaching .patch-Files for some reason, but if you want to apply my commit, just save the code block below to a file and run git am <filename>

From 4017d905d9c787400c1b4c6abb83d787dc0d69fa Mon Sep 17 00:00:00 2001
From: nzbr <mail@nzbr.de>
Date: Thu, 21 Apr 2022 10:29:22 +0200
Subject: [PATCH] rename existing docker option to docker-desktop

---
 configuration.nix          | 2 +-
 modules/docker-desktop.nix | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/configuration.nix b/configuration.nix
index 8ecca30..26f1680 100644
--- a/configuration.nix
+++ b/configuration.nix
@@ -18,7 +18,7 @@ in
     startMenuLaunchers = true;
 
     # Enable integration with Docker Desktop (needs to be installed)
-    # docker.enable = true;
+    # docker-desktop.enable = true;
 
     # Enable native Docker support within NixOS
     # docker-native.enable = true;
diff --git a/modules/docker-desktop.nix b/modules/docker-desktop.nix
index 3e0d200..d822526 100644
--- a/modules/docker-desktop.nix
+++ b/modules/docker-desktop.nix
@@ -1,13 +1,17 @@
 { config, lib, pkgs, ... }:
 with builtins; with lib; {
 
-  options.wsl.docker = with types; {
+  imports = [
+    (mkRenamedOptionModule [ "wsl" "docker" ] [ "wsl" "docker-desktop" ])
+  ];
+
+  options.wsl.docker-desktop = with types; {
     enable = mkEnableOption "Docker Desktop integration";
   };
 
   config =
     let
-      cfg = config.wsl.docker;
+      cfg = config.wsl.docker-desktop;
     in
     mkIf (config.wsl.enable && cfg.enable) {
 
-- 
2.33.1


@nzbr
Copy link
Member

nzbr commented Apr 21, 2022

Renaming the new module to just docker conflicts with the mkRenamedOptionModule in docker-desktop.nix. Also, this would switch anyone who currently uses the docker desktop integration over to docker native, which may not be desired. I prefer we leave docker-desktop and docker-native as two equal options

@ajaxbits
Copy link
Contributor Author

Thanks, @nzbr, I also noticed that error with mkRenamedOptionModule when I made my push. Sorry about that.

Things should be resolved now, as I reverted to stick with the docker-native option. Let me know if you think there's a better name for this option, as I'm not in love with "docker-native."

I also made some misc. wording changes in configuration.nix to clarify things.

@nzbr
Copy link
Member

nzbr commented Apr 21, 2022

I think docker-native is fine, especially as it clearly differentiates the option from docker-desktop. I'll merge this now, if that's fine with you

@ajaxbits
Copy link
Contributor Author

I'll merge this now, if that's fine with you

Sounds great to me!

@nzbr nzbr merged commit 4811cf8 into nix-community:main Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants