feat(preprod): Add snapshots subcommand#3110
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Hey @lcian, are you ready for me to review this, or are you still planning to check the feedback from Bugbot and/or iterate further? |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Looking mostly good!
Most of the comments I left are optional (marked with "l"). The comment about optimizing memory usage (marked "m") can be implemented in a follow-up PR, as we agreed offline.
The two things blocking a merge, from my perspective, are that:
- We are depending on a branch of the
objectstore-clientrepository, rather than a released version. - I am a bit concerned about how we handle the auth token; it seems that we could easily end up logging the token in an error message
It would also be good to fix the hidden file implementation, but I think that is also ok to ship as a follow up
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable autofix in the Cursor dashboard.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
lgtm 🚀
I have added some minor, optional-to-address comments
| height: image.height, | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
@lcian Please consider making an issue so this comment doesn't get lost after we merge the PR
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| width: image.width, | ||
| height: image.height, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Duplicate-content images silently dropped from manifest
Low Severity
When two image files have identical content (same SHA256 hash), both uploads are queued in many_builder, but the second manifest_entries.insert(hash, ...) silently overwrites the first entry's ImageMetadata, including its image_file_name. The manifest sent to the server will have fewer entries than the number of images processed, with no warning to the user. The "Uploaded N image files" message uses image_count (total images) while the manifest may contain fewer entries, creating a silent inconsistency.
| sourcemap = { version = "9.3.0", features = ["ram_bundle"] } | ||
| symbolic = { version = "12.13.3", features = ["debuginfo-serde", "il2cpp"] } | ||
| thiserror = "1.0.38" | ||
| tokio = { version = "1.47", features = ["rt"] } |
There was a problem hiding this comment.
Tokio dependency missing required feature flags for I/O
Low Severity
The tokio dependency declares only features = ["rt"], but the code calls enable_all() on the runtime builder, which conditionally enables I/O and time drivers only if the net and time features are compiled in. Currently this works because reqwest (via objectstore-client) transitively enables these features, but the explicit dependency doesn't document the actual requirements. If the transitive dependency chain changes, enable_all() becomes a no-op for I/O, causing silent runtime failures during HTTP requests.
Additional Locations (1)
5 participants
Updated version of #3049 to discuss and iterate on things.
Notable changes:
shard_indexparameter from the command. I'm not sure what the purpose of that was originally.many(batch) API fromobjectstore_client. All uploads are executed as batch requests, reducing network overhead. Unfortunately, with they way things are implemented now, we will still have to buffer all files in memory before sending the request, as we need to hash their contents to determine the filename. If we could just use the filename as the key in objectstore, it would be much better because that way we could stream the files over.Note that the objectstore endpoint is currently gated by a feature flag, and only enabled for internal orgs/teams.
Ref FS-233