fix(mounts): honor pattern options in mountpoint and s3files#3622
fix(mounts): honor pattern options in mountpoint and s3files#3622mshsheikh wants to merge 2 commits into
mountpoint and s3files#3622Conversation
- Wire `MountpointMountPattern.options` and `S3FilesMountPattern.options` into command assembly so pattern-level settings are actually applied at runtime. - This restores the intended configuration surface for mount-specific overrides while preserving existing runtime config behavior as the fallback path. - No behavior changes for callers that do not use pattern options.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7705469f49
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| region = self.options.region or mountpoint_config.region | ||
| endpoint_url = self.options.endpoint_url or mountpoint_config.endpoint_url | ||
| prefix = self.options.prefix or mountpoint_config.prefix |
There was a problem hiding this comment.
When a shared MountpointMountPattern has defaults and an individual S3Mount/GCSMount also sets region, endpoint_url, or prefix, these lines make the pattern value win at apply time. The provider builders already resolve MountpointMountConfig with the per-mount value first and the pattern value as fallback, so re-reading self.options here silently discards mount-specific overrides and mounts the wrong bucket prefix/endpoint/region in mixed-mount configurations.
Useful? React with 👍 / 👎.
| mount_target_ip = self.options.mount_target_ip or s3files_config.mount_target_ip | ||
| access_point = self.options.access_point or s3files_config.access_point | ||
| region = self.options.region or s3files_config.region |
There was a problem hiding this comment.
If a reused S3FilesMountPattern supplies default mount_target_ip, access_point, or region, any S3FilesMount that sets its own value is still overwritten here because self.options is preferred over the already-resolved S3FilesMountConfig. S3FilesMount.build_in_container_mount_config resolves config with mount-specific values first, so this reapplication sends the mount helper to the default target/access point instead of the per-mount override.
Useful? React with 👍 / 👎.
- Adjust `MountpointMountPattern` and `S3FilesMountPattern` so per-mount config remains the primary source of truth, with pattern options acting only as defaults. - This corrects the precedence order in command assembly and preserves the expected override behavior for mount-specific region, endpoint, prefix, access point, and mount target settings. - No change to public API shape; this only fixes resolution order at runtime.
2 participants
Wire
MountpointMountPattern.optionsandS3FilesMountPattern.optionsinto command assembly so pattern-level settings are actually applied at runtime.This restores the intended configuration surface for mount-specific overrides while preserving existing runtime config behavior as the fallback path.
No behavior changes for callers that do not use pattern options.