fix: skip kv pairs that have already been uploaded #1970
Conversation
|
would be great if someone could verify that the |
| @@ -95,15 +97,23 @@ pub fn directory_keys_values( | |||
|
|
|||
| validate_key_size(&key)?; | |||
|
|
|||
| // asset manifest should always contain all files | |||
| asset_manifest.insert(url_safe_path, key.clone()); | |||
There was a problem hiding this comment.
What happens if there are duplicate keys? Will we upload both and the second gets overridden?
There was a problem hiding this comment.
can there be duplicates? these are based on files on disk no?
There was a problem hiding this comment.
I'm not familiar with this service 😅 if there can't be duplicates it's not worth worrying about.
There was a problem hiding this comment.
i'm not that familiar with it either, but the code suggests that it's iterating over files and creating a bundle to upload. the filepaths which ultimately determine the key & value would be unique
| @@ -64,6 +65,7 @@ pub fn add_namespace(user: &GlobalUser, target: &mut Target, preview: bool) -> R | |||
| pub fn directory_keys_values( | |||
| target: &Target, | |||
| directory: &Path, | |||
| exclude: Option<&HashSet<String>>, | |||
There was a problem hiding this comment.
I think it would be nice to remove the Option - if callers don't want to ignore any keys, they can pass an empty set.
| exclude: Option<&HashSet<String>>, | |
| exclude: &HashSet<String>, |
There was a problem hiding this comment.
create a new HashSet and pass a ref to it, instead of None?
There was a problem hiding this comment.
Yup, exactly: &HashSet::new()
There was a problem hiding this comment.
I don't see the benefit, can you elaborate?
There was a problem hiding this comment.
Option<HashSet> has two ways to represent "don't exclude any files": Some(HashSet::new()) and None. HashSet only has one.
There was a problem hiding this comment.
This also removes the if let Some(...) line below, which decreases the indentation a little.
There was a problem hiding this comment.
hmm. yea, i suppose. None is pretty clear from an intent standpoint. Going to leave as-is, and if its worthwhile we can revisit
| use std::path::Path; | ||
|
|
||
| #[test] | ||
| fn it_can_filter_preexisting_files() { |
There was a problem hiding this comment.
I guess this would be too painful to make work with directory_keys_values for now? Would be good to add back in later with a mock Walk or something.
There was a problem hiding this comment.
Yea, exactly -- the old test really didn't prove much... and with the logic embedded in the other function now it's a bit hairy to test. We'd want to change the implementation to make it testable but since this is just a quick patch I think it's ok.
cc/ @jyn514 (w/r/t your question above)
This adds an optional param to the function which creates the kv pairs for upload. The logic was pulled from the
filter_filesfunction, which itself has been removed (along with its test).The primary motivation here is to skip any kv pair that has already been uploaded (checked by listing keys from the namespace), so we don't grow the
vecso needlessly large.