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.7.0 (package and service) #85842

Merged
merged 2 commits into from Jan 13, 2021

Conversation

iblech
Copy link
Contributor

@iblech 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
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 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?

nixos/modules/services/web-apps/whitebophir.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/whitebophir.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/whitebophir.nix Outdated Show resolved Hide resolved
@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
Member

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

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
@iblech iblech changed the title whitebophir: init at 1.6.0 (package and service) whitebophir: init at 1.6.4 (package and service) Oct 23, 2020
@iblech iblech requested a review from aanderse October 23, 2020 12:27
@iblech iblech changed the title whitebophir: init at 1.6.4 (package and service) whitebophir: init at 1.6.5 (package and service) Jan 6, 2021
@iblech
Copy link
Contributor Author

iblech commented Jan 6, 2021

Just a heads up that I'm still using this package and service on a regular basis and that it seems to work fine. I'll appreciate any guidance on what has to be done in order for this pull request to get merged. :-)

@iblech iblech changed the title whitebophir: init at 1.6.5 (package and service) whitebophir: init at 1.7.0 (package and service) Jan 11, 2021
@iblech
Copy link
Contributor Author

iblech commented Jan 11, 2021

Just a couple hours ago, whitebophir 1.7.0 was released. I took the opportunity of bumping to rebase against current master.

pkgs/servers/web-apps/whitebophir/node-packages.nix Outdated Show resolved Hide resolved
pkgs/servers/web-apps/whitebophir/default.nix Outdated Show resolved Hide resolved
pkgs/servers/web-apps/whitebophir/default.nix Outdated Show resolved Hide resolved
pkgs/servers/web-apps/whitebophir/default.nix Outdated Show resolved Hide resolved
@iblech
Copy link
Contributor Author

iblech commented Jan 12, 2021

@infinisil: Thank you for your thorough review, very much appreciated. I believe I implemented all of your suggestions in the new force-push. :-)

@infinisil
Copy link
Member

Looking good!

@infinisil
Copy link
Member

infinisil commented Jan 12, 2021

Oh one last thing: Organize the commits as:

  • An initial commit for the package (with commit message "whitebophir: init at 1.7.0")
  • Another commit for the NixOS module (with commit message "nixos/whitebophir: init")

(with git rebase -i HEAD~10)

@iblech
Copy link
Contributor Author

iblech commented Jan 13, 2021

@infinisil Just did so. :-)

@infinisil infinisil merged commit 0cd5058 into NixOS:master Jan 13, 2021
@infinisil
Copy link
Member

Nice work!

@iblech
Copy link
Contributor Author

iblech commented Jan 13, 2021

Super awesome, thank you for handholding and merging! :-)

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

Successfully merging this pull request may close these issues.

None yet

4 participants