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

thelounge: init at 3.0.1 #51947

Merged
merged 3 commits into from Jul 23, 2019
Merged

thelounge: init at 3.0.1 #51947

merged 3 commits into from Jul 23, 2019

Conversation

Mrmaxmeier
Copy link
Contributor

@Mrmaxmeier Mrmaxmeier commented Dec 13, 2018 •
edited

Motivation for this change

The Lounge is the official and community-managed fork of Shout - The self-hosted web IRC client.
The main motivation for inclusion to nixpkgs is that I don't really know how to maintain packages out-of-tree (:

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

I've been running this for a few days.

A few things to consider:

  • The service should be compatible with the soon-to-be-released v3.0 of The Lounge. The update to v3 might happen during unrelated rebuilds of nodePackages because the version is not pinned. Not sure if that's ideal. It's in line with shout package though.
  • User-management for private instances is not (yet?) handled by this PR. It might be useful to include a script that sets THELOUNGE_HOME and invokes the thelounge binary as the thelounge user.
  • The shout package and service should probably be marked as deprecated or for removal. Is there a standard way to do this?
  • There might be a more canonical way to handle mostly-known config options.

@Mic92 Mic92 changed the title thelounge: init at v2.7.1 thelounge: init at 2.7.1 Dec 13, 2018
Mic92
Mic92 reviewed Dec 13, 2018
View reviewed changes
nixos/modules/misc/ids.nix Outdated Show resolved Hide resolved
@Mic92
Copy link
Member

Mic92 commented Dec 13, 2018

When removing should, you can add a entry to our 19.03 release notes.

@Mrmaxmeier Mrmaxmeier mentioned this pull request Dec 16, 2018
Closed
@Mrmaxmeier Mrmaxmeier changed the title thelounge: init at 2.7.1 thelounge: init at 3.0.1 Feb 28, 2019
@nornagon
Copy link
Contributor

Is there anything I can do to help out getting this landed?

(And/or, how do I patch this in locally to test it out...?)

infinisil
infinisil requested changes Apr 15, 2019
View reviewed changes
nixos/modules/services/networking/thelounge.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/thelounge.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/thelounge.nix Show resolved Hide resolved
nixos/modules/services/networking/thelounge.nix Show resolved Hide resolved
nixos/modules/services/networking/thelounge.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/thelounge.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/thelounge.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/thelounge.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
infinisil
infinisil requested changes May 14, 2019
View reviewed changes
nixos/modules/services/networking/thelounge.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/thelounge.nix Outdated Show resolved Hide resolved
@Mrmaxmeier
Copy link
Contributor Author

@infinisil I've addressed your review.
Is this ready to be merged?

infinisil
infinisil requested changes Jul 9, 2019
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Other than these little things this looks good to me. I can merge once those are addressed

nixos/modules/services/networking/thelounge.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/thelounge.nix Outdated Show resolved Hide resolved
@Mrmaxmeier Mrmaxmeier force-pushed the init-thelounge branch 3 times, most recently from 2473e45 to 4c44118 Compare July 10, 2019 08:35
@infinisil infinisil mentioned this pull request Jul 10, 2019
Closed
2 tasks
infinisil
infinisil approved these changes Jul 21, 2019
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Alright this looks good to me :)

Now you just have to fix the merge conflict by updating the node set again (node packages are a bit annoying in nixpkgs..)

Feel free to ping me or @Mic92 when ready so we can merge

@infinisil
Copy link
Member

Oh and one more thing: Would be nice if you could split up the commits into adding the package, updating node modules, then adding the NixOS module, in that order

@Mrmaxmeier
Copy link
Contributor Author

@infinisil 🏓

@infinisil
Copy link
Member

NixOS module commits should start with "nixos/thelounge: ", and I'd put both last commits into a single one named "nixos/thelounge: init" (no version because the module is version-agnostic, and no separate commit for the non-dynamic thing because that was just addressing feedback from a review)

@Mrmaxmeier Mrmaxmeier force-pushed the init-thelounge branch 2 times, most recently from 6f223ea to bf03b28 Compare July 22, 2019 18:05
@Mrmaxmeier
thelounge: init node package at v3.0.1
a37b32b
@Mrmaxmeier
regenerate node-packages*.nix for nodePackages.thelounge
09b0041
@Mrmaxmeier
nixos/thelounge: init
37a2f05 
The Lounge is the official and community-managed fork of Shout.
This intends to replace the `shout` service.
@Mrmaxmeier
Copy link
Contributor Author

Ok. I've rebased again and fixed up the commit log.

@infinisil infinisil merged commit 8403187 into NixOS:master Jul 23, 2019
@infinisil
Copy link
Member

Thanks for your patience :)

@Mrmaxmeier Mrmaxmeier deleted the init-thelounge branch August 14, 2022 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@infinisil infinisil infinisil approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Mrmaxmeier @Mic92 @nornagon @infinisil @GrahamcOfBorg