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

Jump to bottom

code-server: init code-server-module #135601

Merged
merged 2 commits into from Nov 4, 2021
Merged

code-server: init code-server-module #135601

merged 2 commits into from Nov 4, 2021

Conversation

stackshadow
Copy link
Contributor

@stackshadow stackshadow commented Aug 24, 2021 •
edited

Motivation for this change
  • an nixos-module was missing for code-server to easy config code-server
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

SuperSandro2000
SuperSandro2000 requested changes Aug 24, 2021
View reviewed changes
pkgs/servers/code-server-bin/default.nix Outdated Show resolved Hide resolved
pkgs/servers/code-server-bin/default.nix Outdated Show resolved Hide resolved
pkgs/servers/code-server-bin/default.nix Outdated Show resolved Hide resolved
pkgs/servers/code-server-bin/default.nix Outdated Show resolved Hide resolved
pkgs/servers/code-server-bin/default.nix Outdated Show resolved Hide resolved
pkgs/servers/code-server-bin/default.nix Outdated Show resolved Hide resolved
pkgs/servers/code-server-bin/default.nix Outdated Show resolved Hide resolved
@mausch
Copy link
Member

mausch commented Aug 26, 2021

really hard to understand and maintain

The difference is, the existing code-server package builds from source and the one in this PR apparently doesn't...
Building from source is generally more flexible, allows patching, etc.

If there are issues updating code-server maybe the maintainers could help? i.e. create one of these issues tagging the maintainers https://github.com/NixOS/nixpkgs/issues?q=is%3Aissue+is%3Aopen+label%3A%229.needs%3A+package+%28update%29%22

Having a module around code-server is a great idea but IMHO it should be done separately.

@stackshadow
Copy link
Contributor Author

really hard to understand and maintain

The difference is, the existing code-server package builds from source and the one in this PR apparently doesn't...
Building from source is generally more flexible, allows patching, etc.

You're right 🤔 maybe i try to update the original code-server-package and transform this PR to an module-only PR.
I tested the module already against the pkgs.code-server and it works

@stackshadow stackshadow marked this pull request as draft August 26, 2021 18:11
@stackshadow
Copy link
Contributor Author

Having a module around code-server is a great idea but IMHO it should be done separately.

So, do you think to create a separate PR for the module or can it stay here with the upgrade of code-server-package ?

@mausch
Copy link
Member

mausch commented Aug 26, 2021

I tested the module already against the pkgs.code-server and it works

Nice!

So, do you think to create a separate PR for the module or can it stay here with the upgrade of code-server-package ?

Maybe leave this PR to be about the module only, since you already got that working against the existing package.

@stackshadow stackshadow changed the title code-server-bin: init at 3.11.1 code-server: init code-server-module Aug 27, 2021
@stackshadow stackshadow marked this pull request as ready for review August 27, 2021 16:32
aanderse
aanderse requested changes Aug 29, 2021
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment •
edited

Choose a reason for hiding this comment

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

Hello. I left a review on your module. If you need any help or have any questions please don't hesitate to ask. I'm always happy to lend a helping hand.

nixos/modules/services/web-apps/code-server.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/code-server.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/code-server.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/code-server.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/code-server.nix Outdated

packages = mkOption {
default = [ ];
defaultText = "[]";
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need that here... 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, its really needed for usage of code-server.
Problem is, that code-server is started by systemd. Lets say we would like to develop in go ( i do this in my day to day job ) problem is, that code-server is started by systemd. In the env of systemd there is no PATH to the go binary, even if its installed as system-package in nixos. Thats why i use the path option in the systemd-service.
I was thinking if i can use environment.systemPackages for that 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, misunderstanding. This option is definitely needed. The defaultText isn't. 😄

nixos/modules/services/web-apps/code-server.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/code-server.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/code-server.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/code-server.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/code-server.nix Show resolved Hide resolved
@aanderse
Copy link
Member

@stackshadow I have pushed a few commits to a branch which you can feel free to include in your PR here: https://github.com/stackshadow/nixpkgs/compare/code-server-bin...aanderse:nixos/code-server?expand=1

Let me know if you have any questions.

@stackshadow
Copy link
Contributor Author

@stackshadow I have pushed a few commits to a branch which you can feel free to include in your PR here: https://github.com/stackshadow/nixpkgs/compare/code-server-bin...aanderse:nixos/code-server?expand=1

Let me know if you have any questions.

Cool thanks, its now more readable :)

aanderse
aanderse requested changes Sep 7, 2021
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

Just a few more small things. Also, please squash this down to 2 commits:

  • adding yourself to the maintainers list
  • adding the module

Thanks!

nixos/modules/services/web-apps/code-server.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/code-server.nix Show resolved Hide resolved
@stackshadow stackshadow force-pushed the code-server-bin branch 2 times, most recently from ee77af8 to 2c88cd4 Compare September 19, 2021 12:51
@stackshadow
Copy link
Contributor Author

Just a few more small things. Also, please squash this down to 2 commits:

* adding yourself to the maintainers list

* adding the module

Thanks!

Was a little bit tricky to split the commit into two, but this train my git muscle :D

@aanderse
Copy link
Member

Looking good. You forgot to actually set yourself as the maintainer, though.

@stackshadow
Copy link
Contributor Author

Looking good. You forgot to actually set yourself as the maintainer, though.

Ahh right, done :D

aanderse
aanderse approved these changes Oct 4, 2021
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Any reason not to merge? Pull the trigger?

@GoogleBot42
Copy link
Contributor

Is this ready to merge?

@stackshadow
Copy link
Contributor Author

I've rebased the branch to fix the conflicts :)

@stackshadow
Copy link
Contributor Author

So for me its ready to merge :)

@aanderse aanderse merged commit 0c5d86b into NixOS:master Nov 4, 2021
@aanderse
Copy link
Member

aanderse commented Nov 4, 2021

Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@contrun contrun contrun left review comments

@aanderse aanderse aanderse approved these changes

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@stackshadow @mausch @aanderse @GoogleBot42 @SuperSandro2000 @contrun @Janik-Haag