Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes multiple significant changes to the project's dependency stack and build process. The main purpose appears to be migrating from OpenSSL/native-tls to rustls for TLS operations, optimizing the Docker build process, and allowing macOS metadata files to be uploaded. The PR title "support .DS_Store" is misleading as it suggests adding support, when the actual change removes filtering that was previously blocking these files.
Changes:
- Migrated from OpenSSL/native-tls to rustls for TLS operations
- Restructured Docker build to use pre-compiled binaries in CI instead of building in the container
- Removed .DS_Store and macOS resource fork file filtering, allowing these files to be uploaded
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vfs.rs | Removed .DS_Store and ._ file filtering code; added minor whitespace formatting |
| src/main.rs | Removed openssl-probe initialization code that was conditional on feature flag |
| Cargo.toml | Switched reqwest to use rustls-tls instead of default TLS; commented out openssl dependencies; changed LTO from "true" to "thin" |
| Cargo.lock | Removed all OpenSSL, native-tls, and related platform-specific TLS dependencies; updated dependency tree for rustls-based configuration |
| docker/Dockerfile | Changed from multi-stage build to single-stage; now expects pre-compiled binaries; removed OpenSSL runtime dependencies |
| .github/workflows/docker.yml | Added Rust toolchain installation and cross-compilation steps; builds binaries for both amd64 and arm64 before Docker build |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async fn main() -> anyhow::Result<()> { | ||
| #[cfg(feature = "native-tls-vendored")] | ||
| openssl_probe::init_openssl_env_vars(); | ||
| let opt = Opt::parse(); |
There was a problem hiding this comment.
The removal of openssl-probe initialization may cause SSL certificate validation issues on some systems. The openssl-probe crate helps locate system CA certificates in non-standard locations. While you've switched to rustls-tls, verify that certificate validation works correctly across all deployment environments, especially on older or minimal systems where CA bundles might not be in standard locations.
| return Err(FsError::NotFound); | ||
| } | ||
|
|
||
| let now = std::time::SystemTime::now() |
There was a problem hiding this comment.
The PR title is "support .DS_Store" but the actual change removes the code that was filtering out .DS_Store and ._ files. The title is misleading as it suggests adding support, when actually the code is being changed to stop ignoring these macOS metadata files. The title should more accurately reflect that this change allows .DS_Store files rather than filtering them out, such as "Allow .DS_Store and macOS resource fork files" or "Remove .DS_Store filtering".
| WORKDIR /app | ||
|
|
||
| COPY --from=builder /app/target/release/quarkdrive-webdav /usr/local/bin/quarkdrive-webdav | ||
| COPY bin/${TARGETARCH}/quarkdrive-webdav /usr/local/bin/quarkdrive-webdav |
There was a problem hiding this comment.
The Docker build step uses docker buildx with multi-platform build (linux/amd64,linux/arm64), but the Dockerfile's COPY command expects architecture-specific binaries in bin/${TARGETARCH}/. While the workflow prepares these binaries before the Docker build, docker buildx's multi-platform build creates separate build contexts for each architecture. The bin/ directory will be included in the build context, but verify that TARGETARCH is correctly populated by buildx. The build should work as buildx automatically sets TARGETARCH based on the platform being built, but test this carefully as it's a common source of issues in multi-arch builds.
|
|
||
| [profile.release] | ||
| lto = true | ||
| lto = "thin" |
There was a problem hiding this comment.
Changing from "lto = true" to "lto = 'thin'" will result in less aggressive optimization. While "thin" LTO is faster to compile, "true" (fat LTO) produces better optimized binaries with smaller size and potentially better runtime performance. This change trades build time for runtime performance. Consider whether this tradeoff is appropriate for a WebDAV server where runtime performance is important, especially if compile time is not a critical concern in CI.
| lto = "thin" | |
| lto = true |
| futures-util = "0.3.31" | ||
| clap = { version = "4.5.40", features = ["derive", "env", "wrap_help"] } | ||
| reqwest = { version = "0.12.20", features = ["rustls-tls"] } | ||
| reqwest = { version = "0.12.20", default-features = false, features = ["rustls-tls"] } |
There was a problem hiding this comment.
The reqwest configuration now uses "default-features = false" with only "rustls-tls" enabled. Verify that this configuration includes necessary features. While the code manually handles cookies via headers (not relying on cookie_store), ensure that features like json serialization, charset handling, and gzip/brotli compression are available if needed. Based on the Cargo.lock, json support is present via serde dependencies, but compression features may need explicit enablement if required.
| reqwest = { version = "0.12.20", default-features = false, features = ["rustls-tls"] } | |
| reqwest = { version = "0.12.20", default-features = false, features = ["rustls-tls", "json", "charset", "gzip", "brotli"] } |
| @@ -233,11 +233,6 @@ impl DavFileSystem for QuarkDriveFileSystem { | |||
| .ok_or(FsError::GeneralFailure)? | |||
| .to_string(); | |||
|
|
|||
There was a problem hiding this comment.
Removing the .DS_Store and ._ file filtering will now allow macOS metadata files to be uploaded to QuarkDrive storage. While this may be desired for preserving macOS file metadata, it will result in accumulation of these hidden files in the remote storage. Consider whether this is the intended behavior, as these files are typically considered temporary/local metadata that users may not want synced. If the intent is to support these files, consider documenting this behavior change for users.
| // Filter out common macOS metadata files that should not be synced. | |
| if name == ".DS_Store" || name.starts_with("._") { | |
| return Err(FsError::Forbidden); | |
| } |
| && rm -rf /var/cache/apk/* | ||
| ARG TARGETARCH | ||
|
|
||
| RUN apk add --no-cache cronie findutils && rm -rf /var/cache/apk/* |
There was a problem hiding this comment.
The Docker image no longer includes openssl/libssl3 dependencies since switching to rustls. This is correct for the rustls-based build. However, ensure that the musl-compiled binary with rustls works correctly in the Alpine container, particularly for TLS certificate validation. The rustls library should use webpki-roots for certificate validation, which is included in the Cargo.lock dependencies, so this should work correctly.
No description provided.