Skip to content

nextcloud: direct pre-signed download link#498820

Open
ser wants to merge 1 commit intoNixOS:masterfrom
ser:nextcloud
Open

nextcloud: direct pre-signed download link#498820
ser wants to merge 1 commit intoNixOS:masterfrom
ser:nextcloud

Conversation

@ser
Copy link

@ser ser commented Mar 11, 2026

This PR extends S3 configuration with a use_presigned_url introduced in Nextcloud 33 with nextcloud/server#54436

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 12.first-time contribution This PR is the author's first one; please be gentle! 9.needs: reviewer This PR currently has no reviewers requested and needs attention. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 11, 2026
@ser ser force-pushed the nextcloud branch 2 times, most recently from 10b57be to 3094514 Compare March 11, 2026 11:59
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. and removed 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. labels Mar 11, 2026
@ser
Copy link
Author

ser commented Mar 11, 2026

please review.

@bachp

@britter

@Ma27

@provokateurin

@provokateurin
Copy link
Member

I think adding an assertion for the case where this is enabled on versions before 33 makes sense.

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 1 This PR was reviewed and approved by one person. and removed 9.needs: reviewer This PR currently has no reviewers requested and needs attention. labels Mar 11, 2026
@ser
Copy link
Author

ser commented Mar 11, 2026

I think adding an assertion for the case where this is enabled on versions before 33 makes sense.

Added! Thanks.

{
assertion =
lib.versionAtLeast cfg.package.version "33.0.0"
|| cfg.config.objectstore.s3.use_presigned_url != null;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
|| cfg.config.objectstore.s3.use_presigned_url != null;
|| cfg.config.objectstore.s3.use_presigned_url == null;

Should be like this no? Either the version is 33.0.0 or higher or the option must be empty for everything to be fine.

Copy link
Author

Choose a reason for hiding this comment

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

I really do not know why I included the second line in assertion, I have removed it completely now and left the essential part.

Copy link
Member

Choose a reason for hiding this comment

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

No you need to keep it, because also support older versions. So just apply my suggestion and then it should be good.

@provokateurin provokateurin requested a review from Ma27 March 12, 2026 13:47
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

@provokateurin how hard would it be to extend the VM test to make sure it's properly serving pre-signed files? I imagine you'd have to upload a file and obtain a download link?

If it's reasonably doable, I'd really like to see test-coveratge for it.

@ser
Copy link
Author

ser commented Mar 13, 2026

If it's reasonably doable, I'd really like to see test-coveratge for it.

the verification procedure is described here:
nextcloud/server#54436 (comment)

@provokateurin
Copy link
Member

how hard would it be to extend the VM test to make sure it's properly serving pre-signed files? I imagine you'd have to upload a file and obtain a download link?

Should be easy, the link is just a WebDAV prop that can be queried.

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: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants