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 option to enable the service for all users #83

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Ten0
Copy link
Collaborator

@Ten0 Ten0 commented Jul 3, 2024

Resolves #69
Resolves #65

What it does:

  • When the services.vscode-server.enableForUsers.enable = true is specified and using nixos-vscode-server as a NixOS module, uses tmpfiles to setup the appropriate symlinks to enable the service for all regular users
  • Adds relevant instructions to the README
  • Also adds instructions to the README to add instructions about how to use nixos-vscode-server as a home-manager module in combination with flakes

@fsnkty
Copy link

fsnkty commented Oct 16, 2024

Just as a sanity check is there a reason the scope of this is only to enable the service for all users?
is enabling the ability to specify the specific users to enable the service on a big change?
thanks for making this PR

@Ten0
Copy link
Collaborator Author

Ten0 commented Oct 16, 2024

No that wouldn't be a big change and it would also make sense.
I have pretty much lost hope that this is ever getting merged though so I'm not investing more time in this, as there is no active maintainer on this project (#67 (comment), but then I got ghosted, which honestly is understandable as the xz story happened at about the same time so they probably got suddenly unadventurous 😅).

I would be willing to merge a PR to this branch to my fork though 😊

@fsnkty
Copy link

fsnkty commented Oct 16, 2024

No that wouldn't be a big change and it would also make sense. I have pretty much lost hope that this is ever getting merged though so I'm not investing more time in this, as there is no active maintainer on this project (#67 (comment), but then I got ghosted, which honestly is understandable as the xz story happened at about the same time so they probably got suddenly unadventurous 😅).

understandable, maybe its time to look into finding the right person to ask about managing nix-community here given they seem both unable to work on the repo and unwilling (?) to allow someone else to.

unfortunate, guess I'll get to solving this elsewhere

@msteen
Copy link
Collaborator

msteen commented Oct 17, 2024

@Ten0 Due to various reasons I don't want to continue working on this project. I asked before (in matrix), without success, but now someone looked into it and turned out I wasn't admin of my own project (explained why I could not find the settings page no matter what I tried). I just invited you as maintainer.

@Ten0
Copy link
Collaborator Author

Ten0 commented Oct 17, 2024

Thanks! 😊
I'll do my best 😊

@moritztim
Copy link

moritztim commented Oct 18, 2024

Hey, thanks so much for taking over! so is this getting merged?

Edit: I just saw that this was a very recent development, so take your time haha

@Ten0 Ten0 force-pushed the enable_for_all_users branch 5 times, most recently from 8d73bcc to be0d05d Compare October 19, 2024 18:10
@Ten0 Ten0 force-pushed the enable_for_all_users branch from be0d05d to b5a62ed Compare October 19, 2024 18:10
@Ten0
Copy link
Collaborator Author

Ten0 commented Oct 19, 2024

@fsnkty does the new proposed interface look good to you?

Copy link

@fsnkty fsnkty left a comment

Choose a reason for hiding this comment

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

documentation comments are suggestions,
expecting the user attribute name to match its .name value however really should be addressed, I don't believe its an uncommon practice

otherwise all seems good to me bar some practices I don't fully understand that aren't related to this PR.

thank you for working on this

modules/vscode-server/default.nix Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
modules/vscode-server/module.nix Show resolved Hide resolved
@Ten0 Ten0 force-pushed the enable_for_all_users branch 2 times, most recently from c0b7edb to d7ad842 Compare October 20, 2024 10:47
@Ten0 Ten0 force-pushed the enable_for_all_users branch from d7ad842 to 725f14e Compare October 20, 2024 10:50
@Ten0 Ten0 requested a review from fsnkty October 20, 2024 11:05
let
forEachUser = ({ path, file }: builtins.listToAttrs
(builtins.map
(username: let user = config.users.users.${username}; in {
Copy link

@fsnkty fsnkty Oct 20, 2024

Choose a reason for hiding this comment

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

I'm still unable to get this to work as expected both config.users.users.foo.name and "bar" fail.
when e.g.. config.users.users.foo.name = "bar"
for reference users.groups.<name>.members handles this correctly, may be useful to take a look at that.
thank you

Copy link
Collaborator Author

@Ten0 Ten0 Oct 21, 2024

Choose a reason for hiding this comment

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

Can you please expand on how it "doesn't work as expected"?

Copy link

Choose a reason for hiding this comment

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

Can you please expand on how it "doesn't work as expected"?

I have config.users.users.main.name = "fsnkty"
setting services.vscode-server.enableForUsers.users = [ config.users.users.main.name ];
breaks this in that

       error: attribute 'fsnkty' missing
       at /nix/store/fbgf31l3gy506di92bbw12aw95jg4cs6-source/modules/vscode-server/default.nix:20:37:
           19|             (builtins.map
           20|               (username: let user = config.users.users.${username}; in {
             |                                     ^
           21|                 name = "${user.home}/${path}";

giving "fsnkty" breaks in the same way, the string "main" can be used but that's non ideal and doesn't behave similarly to any other option that Ive encountered which expects a list of users.

this workaround in using the user attribute name as a string instead of its given name is non ideal because now changing that value is separate from changing it anywhere else a user is specified or named

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.

Declaratively enable user service in NixOS module Instructions unclear with home-manager managed as a flake
4 participants