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

hostctl: init module #249

Merged
merged 7 commits into from
Jan 17, 2023
Merged

hostctl: init module #249

merged 7 commits into from
Jan 17, 2023

Conversation

shyim
Copy link
Contributor

@shyim shyim commented Dec 24, 2022

Fixes #227

Config looks like

hosts = {
  "foo" = "127.0.0.1";
};

It will be setup by entering the shell. I did add a proper "caching" so it's calling only hostctl when required because we need to run sudo.

To deactivate, the user has to run deactivate-hosts in the shell and leave the shell. Next opening of the shell readds them

@shyim
Copy link
Contributor Author

shyim commented Dec 24, 2022

I guess I would add this to devenv up and call the hostctl remove when the command gets canceled using a trap

Copy link
Member

@domenkozar domenkozar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking that hostctl is the implementation detail and the interface should be more like:

hosts = [ "127.0.0.1 foobar" ];

This aligns with how processes work, I don't see any specific reason to have explicit enable. What do you think?


profile = lib.mkOption {
type = lib.types.str;
default = "devenv";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you have two devenvs setting different hosts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last one wins. That's why I added the profileName option 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we use "devenv-${hostHash" for the profile name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to be something unique to this devenv to update also the area in the hosts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"devenv-${builtins.hashString "sha256" env.DEVENV_ROOT}"

src/modules/integrations/hostctl.nix Outdated Show resolved Hide resolved
@shyim
Copy link
Contributor Author

shyim commented Dec 24, 2022

@domenkozar are you okay with the Idea to add pre and post scripts to devenv up. So we are able to start hostctl and stop it after running the processes. I guess without processes there is not usecase for custom hosts 🤔

@domenkozar
Copy link
Member

@domenkozar are you okay with the Idea to add pre and post scripts to devenv up. So we are able to start hostctl and stop it after running the processes. I guess without processes there is not usecase for custom hosts thinking

It's still better to always set the hosts, as someone might rely on that as part of the scripts, etc.

@shyim
Copy link
Contributor Author

shyim commented Dec 25, 2022

The problem is when do we remove the hosts. Currently, I am adding a new command. Feels wrong and can be easily forgotten and then the hosts entries says there 😅.

I would also assume the host entries points to processes running 🤔

@roberth
Copy link
Contributor

roberth commented Dec 26, 2022 via email

@domenkozar
Copy link
Member

Not sure what you'd like me to cover. Do you have a nix issue for it?

------- Original Message -------
On Monday, December 26th, 2022 at 12:16, Domen Kožar @.> wrote: @.(https://github.com/roberth) could you cover this one for the next Nix team meeting? — Reply to this email directly, [view it on GitHub](#249 (comment)), or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

Sorry, I wrote this to the wrong browser tab :)

@domenkozar
Copy link
Member

I'm still unsure how to handle the unloading correctly.

Ideally we'd set a trap function in bash and expose that via exitShell for cleanup.

We'd need to fix the whole env loading business, so probably after #191 and #237

@shyim
Copy link
Contributor Author

shyim commented Jan 2, 2023

Even that would not work. When you have two shells inside it and one leaves the hosts are gone for all 🙈

I still somehow like the Idea to couple it with devenv up

@domenkozar
Copy link
Member

👍 let's couple it with devenv up

@domenkozar
Copy link
Member

@shyim would be great if you plan to finish 🥳

@shyim
Copy link
Contributor Author

shyim commented Jan 11, 2023

Had no time to look currently in 🙈 . maybe weekend

@bobvanderlinden
Copy link
Contributor

Nice! Looks good 👍 I was wondering whether an attrset has been considered for configuration?

{
  hosts = {
    "myhost" = "127.0.0.1";
  };
}

So that it is easier to override/remove. Or are there features of /etc/hosts that I'm overlooking?

@shyim
Copy link
Contributor Author

shyim commented Jan 15, 2023

The only problem is that you want to merge all entries by ip to one line as multiple domains with same IP for some reason slow down DNS resolving on Mac

@domenkozar
Copy link
Member

It should still be possible to convert this into the required format with a bit of transformations.

@bobvanderlinden
Copy link
Contributor

bobvanderlinden commented Jan 16, 2023

This should be able to convert the structure to a hosts file content.

let
  hosts = {
    "myhost" = "127.0.0.1";
    "otherhost" = "127.0.0.1";
    "anotherhost" = "192.168.0.1";
  };
  entries = lib.mapAttrsToList (hostname: ip: { inherit hostname ip; }) hosts;
  entriesByIp = builtins.groupBy ({ ip, ... }: ip) entries;
  hostnamesByIp = builtins.mapAttrs (hostname: entries: builtins.map ({ hostname, ...}: hostname) entries) entriesByIp;
  lines = lib.mapAttrsToList (ip: hostnames: "${ip} ${lib.concatStringsSep " " hostnames}") hostnamesByIp;
in
  lib.concatStringsSep "\n" lines

@shyim
Copy link
Contributor Author

shyim commented Jan 16, 2023

Sooo I have updated it. First of all @bobvanderlinden thanks <3 it works flawlessly

hosts = {
    "foo" = "127.0.0.1";
  };

processes.test.exec = "ping foo";

That was my example nix file.

image

@shyim
Copy link
Contributor Author

shyim commented Jan 16, 2023

We can use the same base to support mkcert to add like SSL to caddy

@shyim shyim requested a review from domenkozar January 17, 2023 12:09
@domenkozar domenkozar merged commit 167b533 into cachix:main Jan 17, 2023
@shyim shyim deleted the add-hostsctl branch January 17, 2023 12:54
@bobvanderlinden
Copy link
Contributor

Awesome 👍 thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Define domains for caddy and add them to /etc/hosts
4 participants