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

nixos/mautrix-whatsapp: init #246842

Merged
merged 6 commits into from Aug 6, 2023

Conversation

frederictobiasc
Copy link
Contributor

Hi,

there already exists two merge requests regarding init of mautrix-whatsapp:

I don't know what the canonical way of dealing with this situation is, but I would like to get a mautrix-whatsapp module introduced. That's why I did my best to pull together all suggestions from those two pull requests and also update them to be compatible with the current version of mautrix-whatsapp. Major changes were in the log configuration.

I'm currently running this configuration successfully on my server.

Description of changes

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, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.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.

Vskilet and others added 4 commits August 2, 2023 21:06
Import from PR NixOS#176025

Co-authored-by: Luflosi <Luflosi@users.noreply.github.com>
This contribution applies suggestions made by Luflosi in
NixOS#176025 (comment)
as well as some general refactoring.

Co-authored-by: Luflosi <Luflosi@users.noreply.github.com>
This contribution applies Example 32 (conventional settings option) from
[nixpkgs](https://nixos.org/manual/nixos/stable/#sec-settings-nix-representable).
@frederictobiasc
Copy link
Contributor Author

@NickCao ping

As suggested by @NickCao this commit moves the defaults back to the
options. Only `homeserver.domain` stays in the config section since the
documentation module does not support referencing attributes of other
modules.
@NickCao NickCao merged commit 90c77d8 into NixOS:master Aug 6, 2023
4 of 5 checks passed
@frederictobiasc frederictobiasc deleted the mautrix-whatsapp-module branch August 6, 2023 09:22
@Luflosi
Copy link
Contributor

Luflosi commented Aug 7, 2023

It would have been nice if you had posted a comment in the other PRs so that everyone there would have gotten a notification. The only way I noticed this PR is that I rebuilt my system and the options introduced by this PR collided with the ones that I defined.

I think you made a small mistake by defining appservicePort. If a user changes the port by setting appservice.port, then appservice.address will still contain http://localhost:29318. I don't know what happens with this discrepancy but I think this should not happen. And it did not happen in a previous commit in this PR.

@Luflosi
Copy link
Contributor

Luflosi commented Aug 7, 2023

I'll create another PR soon™️ to address this and a few other small things I found.

Thank you for finally getting this module into NixOS!

@Vskilet
Copy link
Contributor

Vskilet commented Aug 8, 2023

Thanks a lot for your work and sorry for my slow replies !

Someone has test https://gitlab.com/coffeetables/nix-matrix-appservices/-/tree/main ? It seems that would be nice to integrate #114419

Have great day

@eclairevoyant
Copy link
Member

@frederictobiasc I don't see your name in the all-maintainers list. Can you add this in another PR, following these guidelines: https://github.com/NixOS/nixpkgs/blob/master/maintainers/README.md

@frederictobiasc
Copy link
Contributor Author

@eclairevoyant Hi and please excuse the delay. Thank you for spotting this. I created a PR: #252615

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

5 participants