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

whitebophir: init at 1.6.0 (package and service) #85842

Open
wants to merge 6 commits into
base: master
from

Conversation

@iblech
Copy link
Contributor

iblech commented Apr 23, 2020

Motivation for this change

There are many online collaborative whiteboard websites; this pull request packages one which I particularly like. It's simple but not trivial, easily hackable, and free.

This pull request currently targets a specific Git commit of the upstream repository, as the most recent release would require a ugly postInstall hook to fix up hard-coded paths in the source. The package derivation is loosely based on the derivation for cryptpad.

This is my first service addition, hence I'd appreciate feedback. Is there some addition you'd want to see before you deem it fit for merging?

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@iblech
Copy link
Contributor Author

iblech commented May 4, 2020

I just bumped this to the most recent version, 1.4.0. :-)

@iblech iblech changed the title whitebophir: init at 1.1.0 (package and service) [RFC] whitebophir: init at 1.4.0 (package and service) May 4, 2020
@iblech iblech changed the title whitebophir: init at 1.4.0 (package and service) whitebophir: init at 1.5.0 (package and service) May 17, 2020
@iblech
Copy link
Contributor Author

iblech commented May 17, 2020

Bumped to the new version 1.5.0. :-) @Infinisil, did you get around reviewing this pull request? I'm happy to incorporate any changes :-)

Copy link
Contributor

aanderse left a comment

Just a quick review of the module (I don't review node packages, not familiar with it). Looking good 👍

Is the idea to put this behind a reverse proxy or something?

in {
options = {
services.whitebophir = {
enable = mkOption {

This comment has been minimized.

Copy link
@aanderse

aanderse May 26, 2020

Contributor

mkEnableOption might be nice here.

This comment has been minimized.

Copy link
@iblech

iblech May 28, 2020

Author Contributor

Thanks for the pointer! Fixed in next push.

'';
};

package = mkOption {

This comment has been minimized.

Copy link
@aanderse

aanderse May 26, 2020

Contributor

Is there any variation in what a user might provide here? Maybe this option isn't needed.

This comment has been minimized.

Copy link
@iblech

iblech May 28, 2020

Author Contributor

I got the impression that this is the way it's usually done (for instance cryptpad is the same). I imagine this can be useful if you want to override the service with a custom version of whitebophir, which might actually hapen because whitebophir is so easily hackable. :-)

This comment has been minimized.

Copy link
@aanderse

aanderse May 28, 2020

Contributor

If there are multiple package versions available, build time configuration options, or it is common to specify a custom package this is convenient, otherwise an overlay is generally the preferred way to do this in my opinion.

};

port = mkOption {
type = types.int;

This comment has been minimized.

Copy link
@aanderse

aanderse May 26, 2020

Contributor

There is a types.port.

This comment has been minimized.

Copy link
@iblech

iblech May 28, 2020

Author Contributor

Thanks, fixed in next push.

};

serviceConfig = {
Type = "simple";

This comment has been minimized.

Copy link
@aanderse

aanderse May 26, 2020

Contributor

This is the default and could be omitted. Either way.

This comment has been minimized.

Copy link
@iblech

iblech May 28, 2020

Author Contributor

Thanks, fixed in next push.

@iblech
Copy link
Contributor Author

iblech commented May 28, 2020

Just a quick review of the module (I don't review node packages, not familiar with it). Looking good

Thank you for your encouragement and for for taking the time to review this pull request! I believe I adressed all your concerns. :-)

Is the idea to put this behind a reverse proxy or something?

Yes, exactly. Originally I wanted to include example nginx configuration settings in the documentation, but figured that I'd first wait for a review in order to resolve all concerns in one go.

@aanderse
Copy link
Contributor

aanderse commented May 28, 2020

Thanks for the clarification. Let's find someone to review the package and then you can add the documentation. Great work!

@nixos-discourse
Copy link

nixos-discourse commented May 28, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/171

@iblech iblech changed the title whitebophir: init at 1.5.0 (package and service) whitebophir: init at 1.6.0 (package and service) Jun 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.