Skip to content

nixos/cryptpad: init, cryptpad: init at 2024.6.0#251687

Merged
drupol merged 3 commits intoNixOS:masterfrom
martinetd:cryptpad
Jul 26, 2024
Merged

nixos/cryptpad: init, cryptpad: init at 2024.6.0#251687
drupol merged 3 commits intoNixOS:masterfrom
martinetd:cryptpad

Conversation

@martinetd
Copy link
Copy Markdown
Member

@martinetd martinetd commented Aug 27, 2023

Things done

cryptpad removed its dependency on bower and can be sanely packaged again.
Add the package and module back.
The old module almost didn't do anything and there was no test so this really is a full rewrite and should be considered as such.

I have a few remarks/questions I'll post inline

cc people who were involved in this or earlier versions of the modules:
@Pamplemousse @DavHau @xeji @blitz @zimbatm

Link: cryptpad/cryptpad#295

fixes #217231

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • 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.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog This PR adds or changes release notes 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 27, 2023
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is a bit ugly and fragile -- for example enabling DynamicUsers makes the directory unreadable to nginx. Might be work-aroundable with BindReadOnlyPaths in the service instead... but it works for default config, which will do for now

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's do small steps: let's get this somewhere and let's do follow up to stabilize further hardening, etc.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

left a comment in the code here but not quite sure what to do about ACME. Checking if acceptTerms seems safe enough to do the right thing, but it might be a bit surprising... jitsi for example just sets enableACME to mkDefault true so this is slightly better?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That sounds good to me: if someone generally accepts ACME's terms, we might as well secure their cryptpad install by default.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not really, no, I'm not aware of module doing this by default, some people may accept terms and did not set the DNS record yet, so please, just let the user enable ACME explicitly.

Copy link
Copy Markdown
Member Author

@martinetd martinetd Sep 2, 2023

Choose a reason for hiding this comment

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

I wanted to avoid making the user write their domain multiple times (once in cryptpad.config.httpUnsafeOrigin/httpSafeOrigin and once here) -- would it make sense to expose nginx.virtualhosts.<domain> as cryptpad.nginxVhost/nginxSandboxVhost or something similar?

@martinetd martinetd force-pushed the cryptpad branch 2 times, most recently from a34deef to f7fe4dc Compare August 27, 2023 00:53
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Aug 27, 2023
Copy link
Copy Markdown
Member

@Pamplemousse Pamplemousse left a comment

Choose a reason for hiding this comment

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

Looks good to me overall.

I would like to try out the module locally, but I am having (probably unrelated) issues. Hopefully I will solve that and approve this WE.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That sounds good to me: if someone generally accepts ACME's terms, we might as well secure their cryptpad install by default.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This TODO should be removed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

huh it ended up in a weird place somehow, sorry.

I'd rather keep it or something similar somewhere: right now if someone changes the paths in services.cryptpad.config.blockPath for example the nginx config we have here won't be correct -- we'd need to include a default value in our generated file and use it in the generated nginx config to have something robust.

I don't think it's really useful to do, so I didn't, but it's something to keep in mind if it comes up.
I'll move this comment to the config freeform when this discussion is closed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't see a services.cryptpad.config.blockPath, so I don't quite understand the issue you are describing...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Right, so, "blockPath" is a setting in the config/config.example.js of the cryptpad repo: one can change this through the services.cryptpad.config freeform if they want to... But it wouldn't work, because we hardcoded /var/lib/cryptpad/block in the nginx config.
(It's not in the default values because it's not really needed -- we use the implicit defaults in cryptpad itself for the nodejs service)

Point being, I don't think it's obvious that the nginx config we use depends on the default values, so we probably ought to keep it as a comment somewhere, until someone comes along and parametrizes the config (in case of blockPath it's just a single word to change in the config, so it's not that difficult per se, but there's a dozen fooPath variables in the cryptpad config so doing everything would be a bit of work for little value in my opinion)

I'll move the comment to the end of the freeform and reword it a bit to make it more clear

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Couldn't the module set services.nginx.enable = true; if cfg.nginx.enable?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In my nixos config I also have a few other settings like recommendedTlsSettings and others; I'm not sure how much we'd want to include in the cryptpad module or not? e.g. is nginx safe without these recommended settings? (and if not why do these default to false!)

Looking for nginx.enable in nixos/modules we have both kinds of services -- sourcehut for example has an assert like this, and nextcloud has services.nginx.enable = mkDefault true; some have a straight = true without mkDefault, some use it without checking... Some also set their own 'nginx' option to config.services.nginx.enable.

My personal preference is that one 'config.services.foo' shouldn't enable not obviously related services automatically, so either this assert or defaulting services.cryptpad.nginx to services.nginx.enable, but I don't have any strong feeling either way so happy to set this here instead as you suggest. (read: will do if you confirm)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMHO, the way we do it in other modules is to have a portal from cfg.nginx inside a virtual host fragment of configuration.

This way, you can do things like services.cryptpad.nginx.locations."/".xyz, etc. and act upon the configuration from there.

It makes a lot of sense because then you can extend the NixOS module with any web server as long as you have the internal domain knowledge.

Conclusion: It should not enable NGINX, it should not assert NGINX is enabled. As it is in other modules.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do you have a typical module that does this?
I did a quick look around before writing this and there really is a bit of everything, I didn't see any common pattern. If you're aware of one, pointing me to a "typical" (or ideal) module would be great!

@martinetd
Copy link
Copy Markdown
Member Author

I would like to try out the module locally, but I am having (probably unrelated) issues. Hopefully I will solve that and approve this WE.

Thanks for the review!
I have this in my /etc/nixos so there's no hurry on my end; let's wait until you can test and we agree on small details :)

@Pamplemousse
Copy link
Copy Markdown
Member

TL;DR: it works!

I upgraded my instance from 4.11 to what this PR provides (5.4). Instance is running fine, and documents were conserved.
However, I got messages akin to "This document has been upgraded. Edit is not possible until a registered user opens it" (or alike, I didn't think of writing it down!), forcing me to create a user to be able to edit existing "anonymous" documents.

@Pamplemousse
Copy link
Copy Markdown
Member

Pamplemousse commented Sep 1, 2023

Another thing to note: here's the report of the /checkup endpoint of my instance:

checkup report

This instance is running CryptPad 5.4.0.

Page content was served by "nginx".

You appear to be using a firefox/gecko browser on UNIX to view this page.

52 / 57 tests passed.

Details found below

This instance does not provide a valid adminEmail which can make it difficult to contact its adminstrator to report vulnerabilities or abusive content. This can be configured on your instance's admin panel. Use the provided Flush cache' button for this change to take effect for all users.
Test number14
Returned value
This instance's encrypted support ticket functionality has not been enabled. This can make it difficult for its users to safely report issues that concern sensitive information. This can be configured via the admin panel's Support tab.
Test number15
Returned value
This instance has not been configured to support web administration. This can be enabled by adding a registered user's public signing key to the adminKeys array in cryptpad/config/config.js. Changes to cryptpad/config/config.js will require a server restart in order for /api/config to be updated.
Test number16
Returned valuefalse
No terms of service were specified. This link will be included in the home page footer and 'About CryptPad' menu. It's advised that you either provide one or disable registration. See the relevant documentation about how to customize CryptPad's terms value.
Test number37
Returned value
No privacy policy was specified. This link will be included in the home page footer and 'About CryptPad' menu. It's advised that you either provide one or disable registration. See the relevant documentation about how to customize CryptPad's privacy value.
Test number39
Returned value

Copy link
Copy Markdown
Member Author

@martinetd martinetd left a comment

Choose a reason for hiding this comment

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

TL;DR: it works!

Great!
I also had to update a few documents but I was logged in so it was transparent to me, it's good to know that anonymous users can't do it -- I'll write it in the release notes

Another thing to note: here's the report of the /checkup endpoint of my instance:

sounds coherent with what I have.
My instance has registration disabled so it's not showing the privacy policy etc, but I have an error on a check that only works with registration enabled + the same warnings about support tab etc.
Support tab requires some setup on the server I haven't looked at, that'll probably require some adjustment if someone wants it to work.

I'm a bit out of time for today but to recap:

  • I'll adjust the few comments I replied about just now
  • What do you think about nginx? I'd tend to keep the assert but make services.cryptpad.nginx default to services.nginx.enabled?

Anything else?
I'll find some time to update this tomorrow

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Right, so, "blockPath" is a setting in the config/config.example.js of the cryptpad repo: one can change this through the services.cryptpad.config freeform if they want to... But it wouldn't work, because we hardcoded /var/lib/cryptpad/block in the nginx config.
(It's not in the default values because it's not really needed -- we use the implicit defaults in cryptpad itself for the nodejs service)

Point being, I don't think it's obvious that the nginx config we use depends on the default values, so we probably ought to keep it as a comment somewhere, until someone comes along and parametrizes the config (in case of blockPath it's just a single word to change in the config, so it's not that difficult per se, but there's a dozen fooPath variables in the cryptpad config so doing everything would be a bit of work for little value in my opinion)

I'll move the comment to the end of the freeform and reword it a bit to make it more clear

Copy link
Copy Markdown
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

NGINX configuration is an impeding maintenance disaster IMHO. We ought to separate CryptPad specific needs (and abstract over them in the right way, including changes inside the NGINX module, I am a maintainer of it and can help towards that) and stuff that a user can enable: HSTS, OSCP stapling, TLS hardening, etc.

@RaitoBezarius
Copy link
Copy Markdown
Member

Overall, an awesome news, but there's some work to do in the NixOS module to get its complexity down IMHO :).

Adopting RFC42 could help drastically here, cutting down NGINX complexity and pushing it on the user might be a good idea here too.

Thank you for doing this and I am available (let's say: starting the 6th) to help you land this.

Copy link
Copy Markdown
Member Author

@martinetd martinetd left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your review and offer to help! guidance much appreciated, especially for the nginx bit.

Adopting RFC42 could help drastically here, cutting down NGINX complexity and pushing it on the user might be a good idea here too.

I thought RFC42 was pretty much what I did with cryptpad.config being a freeform -- I'll need to look it up again. Or was this about nginx's config?
I left a bigger comment about nginx on the SSL params things so let's continue that discussion here to keep it threaded

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do you have a typical module that does this?
I did a quick look around before writing this and there really is a bit of everything, I didn't see any common pattern. If you're aware of one, pointing me to a "typical" (or ideal) module would be great!

Copy link
Copy Markdown
Member Author

@martinetd martinetd Sep 2, 2023

Choose a reason for hiding this comment

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

I wanted to avoid making the user write their domain multiple times (once in cryptpad.config.httpUnsafeOrigin/httpSafeOrigin and once here) -- would it make sense to expose nginx.virtualhosts.<domain> as cryptpad.nginxVhost/nginxSandboxVhost or something similar?

@martinetd
Copy link
Copy Markdown
Member Author

Update from cryptpad/cryptpad#1213 : they're dropping their complex nginx config in favor of one that just forwards everything to the node process.

I feel it's a bit of a shame, but that'll definitely be much easier to write "properly", so I'll update this PR with that as soon as I have some time (likely next weekend).
Most questions left in previous comment still stand for when you have time to answer, no hurry.

@martinetd
Copy link
Copy Markdown
Member Author

Sorry for the delay, I had mostly forgotten about this and have been keeping myself busy otherwise so might not have time to finish shortly...

Regarding my last comment, I was waiting for feedback from upstream regarding the default config but they're really really bad at replying to github so never got any answer; they reverted the "simpler nginx config" shortly after committing it and now there's both configs in their documentation.
Basically, we get to pick between either proxying everything through node, or keeping the complex config that does the bulk of simple files sending through nginx. I guess we should strive to keep the efficient config if we can, but if the simple config works it could work as a first step.

With that in mind, what do you think @RaitoBezarius ?
I don't think there's any hurry so while I might not have much time right now, I'll be happy to work with you to convert the horrible nginx config text block as something more dynamic, but I could use some pointers to start with (see previous questions) when you have time to answer.
If you don't have time either (let's say until the end of the year), let's just make this simple and proxy it all for now and I'll open a follow-up PR later™

Cheers.

@RaitoBezarius
Copy link
Copy Markdown
Member

If you don't have time either (let's say until the end of the year), let's just make this simple and proxy it all for now and I'll open a follow-up PR later™

Sounds good for now for me. (unfortunately always too many things to do…)

@michaelshmitty
Copy link
Copy Markdown
Contributor

michaelshmitty commented Dec 18, 2023

Thanks for the effort to bring Cryptpad back to NixOS, @martinetd! I made a feeble attempt last year but I couldn't even get the package to build with my limited Nix skills at the time. I've met one of the Cryptpad developers last month at NGI Forum 2023 in Brussels and they said that they indeed updated their build system, nice!

So I read up on the PR here and saw the discussion on the Nginx configuration bit. By chance I happen to have just done a clean Cryptpad install on a Debian server last week and I remember seeing the notes about Nginx in their documentation. There are indeed 2 ways to put Cryptpad behind Nginx now. From the Cryptpad documentation:

  1. CryptPad’s application server handles active connections via websocket and serves static assets (HTML, Javascript, CSS, etc.). This basic configuration is designed to be very easy to configure for small to midsize instances (up to 3000 concurrent users).
  1. In a production environment, the development team recommends Nginx with our advanced example configuration for the following reasons:
  • Scale to many more users by serving static content with a more scalable web-server instead of the single-threaded NodeJS web-server that is built-in
  • Allow the application server to focus exclusively on handling websocket connections

I went with 1. (up to 3000 concurrent users) on the Debian server because it was for a relatively small setup. But to be honest that's what I often need Cryptpad for. I'd say large scale Cryptad setups (+3000 concurrent users) are probably less common than smaller scale setups. So I would recommend to go forward with the simple Nginx configuration and get this PR moving so that Cryptpad gets back into NixOS. Then future fixup PRs can add the more complicated Nginx configuration.

All that said. Can I do anything to help? I'm testing out this PR on an instance of my own. What can we do to move this forward?
Next I will read up on RFC 42 and I concur with @RaitoBezarius to further implement their comments. Thanks again. Let me know. Cryptpad is awesome!

@michaelshmitty
Copy link
Copy Markdown
Contributor

I've hacked on this for a couple of hours, but it's getting pretty late here and I'm tired. I'd love to help out and polish this PR a bit more. I'm not sure what the best way to collaborate on this is. I think I've been able to clean up some of the code here with the simplified Nginx config, but I wonder what the best way to share is. Because I don't think there's a way I can add commits to this PR. Maybe I have to open a PR at the source at https://github.com/martinetd/nixpkgs/tree/cryptpad? Or should I just dump code suggestions in a review? What do you think?

I'm definitely not done with my incantation so I could continue to work on it over the next days / weeks and then come back here when I feel it looks presentable. Then you can have a look and decide what to do with it?

@michaelshmitty
Copy link
Copy Markdown
Contributor

michaelshmitty commented Dec 18, 2023

Some findings:

  • Websockets don't seem to get used / configured with the simplified Nginx config. I went to check in the setup on the Debian server I did last week and there's no configuration of websockets whatsoever yet Cryptpad works just fine (in page updates, notifications, all working). And I followed the official documentation for that install.
  • You can't have two virtualhosts with ACME for Cryptpad because that will generate two separate certificates. It's in the Cryptpad docs somewhere but you must generate 1 certificate with multiple names. There is an easy way to fix that using useACMEhost though.
  • Use of ssl_dhparam in nginx config should be checked / compared with what is done in sslDhparam
  • Use of resolver in nginx config should be checked / compared with what is done in resolver
  • ...

From what I understand in https://github.com/NixOS/rfcs/blob/45b76f20add8d7e0e1d0bc0eed9357b067c899a6/rfcs/0042-config-option.md it seems like the idea is to have as few settings available as possible. I like that idea. There's not much Cryptpad needs to get going, so fewer options better. I wouldn't go as far as to create a webserver config abstraction to support other webservers. I generally prefer to stick to using what upstream supports.

@martinetd
Copy link
Copy Markdown
Member Author

Sorry for the delay in updating this; I think I've covered everything:

  • updated the package to the latest release; the httpSafePort patch conflicted with a comment update but meanwhile I've clarified with upstream we don't actually need it in most case (basically if the second domain is properly set); the config item is still broken so I've opened env.js: fix httpSafePort handling cryptpad/cryptpad#1571 but we could live without it
  • module-side, they've strengthened the requirement on the websocket so I added a section in the nginx proxy for it as per their example
  • I removed the confinement option and just enabled it; nixos/radicle: init services #314440 also added a service with confinement enabled so let's bite the bullet and that'll be less burden on maintenance. If it turns out to be a problem I'll re-add the option, but I'd expect it to just work for everyone.
  • some small details like nginx recommended proxy settings were enabled; they're set in the example cryptpad config from upstream so we probably want them here too.

I've also updated my own instance to this PR and was able to access old data, create new files etc fine as expected. the checkup only fails HSTS but we don't have any nginx config that sets it so I'll let that fail.

So hopefully this is now ready!

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 19, 2024
@martinetd martinetd force-pushed the cryptpad branch 3 times, most recently from 7822f37 to 4f943fa Compare July 20, 2024 12:02
@martinetd
Copy link
Copy Markdown
Member Author

martinetd commented Jul 20, 2024

I got nerd-snipped into adding some systemd hardening options to the service (then drifted into #328648 because SocketBindDeny=all didn't work...); shouldn't change anything user-facing but I've kept that as a separate commit to easily revert just in case.

(At least no problem on my server, and I've also confirmed disabling blockDailyCheck also still works (EDIT: it connected but didn't validate TLS.. probably fixed now but the thing doesn't log errors so /shrug) even if I'm not sure who would do that... fun fact, if you disable it in the nixosTests then cryptpad tries to connect to the accounting server before eth0 is up so that telemetry request never goes anywhere unless you restart the service later (or make the service depend on having a default route); I'll pretend I didn't see that)

Really won't touch this anymore now.

@martinetd martinetd requested a review from a team as a code owner July 20, 2024 19:39
@martinetd martinetd removed the request for review from a team July 20, 2024 19:41
@martinetd
Copy link
Copy Markdown
Member Author

(sorry to systemd folks, pushed the bpf fix by mistake here and removed it but I assume the harm is done and you got cc'd :/)

Copy link
Copy Markdown
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Diff LGTM, nice work, I'm glad to see it back in nixpkgs !

CC @michaelshmitty

@wegank wegank dismissed RaitoBezarius’s stale review July 23, 2024 14:01

Resolved: NGINX configs guarded by configureNginx, RFC42 adopted

@drupol drupol changed the title cryptpad: re-add package/module nixos/cryptpad: init, cryptpad: init at 2024.6.0 Jul 24, 2024
drupol
drupol previously requested changes Jul 24, 2024
Copy link
Copy Markdown
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Do you mind rewording your commit log messages accordingly?

For example, there's no need to mention that it has been added back.

Just use the regular commit message as if it was the first time it is added.

Something like:

  • nixos/cryptpad: init
  • cryptpad: init at 2024.6.0

This re-adds the cryptpad package that was removed due to bower
packaging being insane.

upstream improved that, so add the package back.

Link: cryptpad/cryptpad#295
Co-authored-by: Pol Dellaiera <pol.dellaiera@protonmail.com>
martinetd and others added 2 commits July 24, 2024 16:17
This is a full rewrite independent of the previously removed cryptpad
module, managing cryptpad's config in RFC0042 along with a shiny test.

Upstream cryptpad provides two nginx configs, with many optimizations
and complex settings; this uses the easier variant for now but
improvements (e.g. serving blocks and js files directly through nginx)
should be possible with a bit of work and care about http headers.

the /checkup page of cryptpad passes all tests except HSTS, we don't
seem to have any nginx config with HSTS enabled in nixpkgs so leave this
as is for now.

Co-authored-by: Pol Dellaiera <pol.dellaiera@protonmail.com>
Co-authored-by: Michael Smith <shmitty@protonmail.com>
cryptpad is not directly exposed to the network, but has plenty that can
be hardened more properly, so fix that.
@martinetd
Copy link
Copy Markdown
Member Author

Fixed the subject line; I think it still makes sense to mention it's a re-add in the details but that probably doesn't bother anyone if I ramble a bit there.

I'm not sure if you meant I should also squash the hardening patch; I'd also prefer to keep it separate in case the hardening causes problems for anyone as I haven't left my instance running with these hardened settings for long enough to be confident all corner cases work properly and it could be helpful to just revert it, but will squash if you think it's better.

@drupol
Copy link
Copy Markdown
Contributor

drupol commented Jul 24, 2024

Thank you!

Opinionated thought: I don't believe mentioning that it has been removed then added again in the commit log message is relevant. However, mentioning it in the commit log description and PR makes sense.

@martinetd
Copy link
Copy Markdown
Member Author

Hmm, I guess that if the file name stays the same doing a git log by path would easily find the removal commit again, but in this case for the package the path isn't the same so one wouldn't stumble on the old package by chance so one wouldn't notice it's a re-add and try to compare with the old package (although to be fair it's different enough that it probably wouldn't be of any help)

For the module git log would yield the removal and old commits, at which point I think being explicit that it's a full rewrite that has nothing to do with the old version is also helpful.
(... and actually now I think about it even if the content was close I'd do something like a revert first and then add whatever fix is needed as a separate commit, so it's also mentioning that it's a re-add indirectly...)

Anyway, in general I'd rather be more verbose than not enough in commit messages -- if what was in commit messages wasn't enough one can always find the PR, but the first place I look is commit messages and if that has enough information I'll be more than happy to not have to open firefox.
So back to square one, sorry for being insistent but if you still want me to reword if as a new module I'll do that later tonight, just say. I don't have to agree if that helps get the module merged :)

@drupol
Copy link
Copy Markdown
Contributor

drupol commented Jul 24, 2024

No worries, we are just discussing, and it's totally fine to not agree on everything.

The current state of things in this PR is good, I wouldn't change anything.

@fricklerhandwerk fricklerhandwerk dismissed drupol’s stale review July 25, 2024 17:08

everything addressed

@drupol drupol merged commit ceda66b into NixOS:master Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (new) This PR adds a module in `nixos/` 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 8.has: tests This PR has tests 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Package request: Cryptpad