[upload] remove --force-unencrypted flag - #719
Conversation
cd3003d to
79d6831
Compare
79d6831 to
a78d259
Compare
98f8ecb to
270d596
Compare
jhagberg
left a comment
jhagberg
left a comment
There was a problem hiding this comment.
go vet, go test ./upload/... and golangci-lint run ./upload/... are all clean on this branch.
One thing I could not anchor inline because the line is outside the diff. After this PR no test reaches fs.Reader = f at upload.go:203. I checked with a mutation: replacing it with panic("MUTANT") leaves the whole suite green on this branch, while the same mutation on main fails in TestUploadWithEncryptionRecursive. That is the plain sda-cli upload file.c4gh path, and it is the one that regressed in #682 and #689, so a future flexible-checksum style regression on the seekable *os.File body would ship unnoticed. TestUploadRecursive and TestUploadWithEncryptionRecursive also drive identical code now, so repointing TestUploadRecursive at an already-encrypted fixture with no key flag would close the coverage hole and remove the duplication in one change. Line 380 already builds such a fixture.
On the removal itself, --force-unencrypted disappears with no deprecation path, so an existing script gets cobra's generic "unknown flag" with nothing pointing at --encrypt-with-key. MarkDeprecated or MarkHidden plus an explanatory error would give people one release to migrate. Related, none of the commits carry ! or a BREAKING CHANGE: footer, so semver tooling reading the log will not see this as the interface break it is.
Two small ones. The tests that set --encrypt-with-key write checksum_{un,}encrypted.{md5,sha256} into upload/, and this PR takes that from two tests to about eleven. TearDownTest cleans them up, but a panic or a timeout leaves four untracked files behind, so two lines in .gitignore would help. And the README:501 whitespace fix in the htsget section is unrelated to this change, which matters a bit more than usual here since #720 has to carry the same hunk across.
Also 270d596 is typed feat but only touches upload_test.go. The repo log already uses test(...) and refactor(...) elsewhere, so test(upload): would match.
jhagberg
left a comment
jhagberg
left a comment
There was a problem hiding this comment.
Thanks, the IsCrypt4GHFile extraction is what I was hoping for, and io.ReadFull with the short-file case folded into false, nil is the right call. Splitting the partial-upload one out into #721 works for me.
Three things from my earlier summary are still open, all your call. The plain upload path still has no test: I re-ran the mutation on f813e3e and fs.Reader = f at upload.go:191 can still be replaced with panic("MUTANT") with the suite green. Repointing TestUploadRecursive at an already-encrypted fixture with no key flag would close it. --force-unencrypted still gives Error: unknown flag: --force-unencrypted with nothing pointing at --encrypt-with-key, and no commit carries ! or a BREAKING CHANGE: footer. And the four checksum_*.{md5,sha256} files the --encrypt-with-key tests drop in upload/ are still not in .gitignore.
Small one in the new code: checkFiles wraps an error that IsCrypt4GHFile already wrapped, so a read failure reads error reading input file x, reason: error reading header of x: ....
| p := mpb.New() | ||
|
|
||
| f, err := os.Open(path.Clean(filename)) | ||
| f, err := os.Open(filepath.Clean(filename)) |
There was a problem hiding this comment.
While you were in here with path vs filepath: line 145 has the same mix-up in the other direction. filepath.Clean(config.AccessKey+"/"+listPrefix) gets compared against an S3 key, and S3 keys always use /. On Windows filepath.Clean returns dummy\somedir\file.c4gh, so fileExists is false for every file, the already-uploaded guard never fires and --continue re-uploads everything it should skip. The rest of the file already guards against this with filepath.ToSlash at 307, 326, 396 and 411.
Nothing catches it either. I simulated the Windows result by swapping / for \ on that line and the upload suite stays green, and the only --continue coverage is 20_upload.sh, which runs ubuntu-only. Pre-existing, so a separate issue is fine by me if you would rather keep this PR to the flag removal.
The flag --force-unencrypted is removed so that we don't get in trouble with the Data Protection Agency or Ethics. BREAKING CHANGE: Removed the --force-unencrypted flag from the upload command.
- remove the section about `--force-unencrypted` - remove the section `Features for testing` since it has no content at all now
- update existing unit tests due to removal of --force-unencrypted - add unit test to verify upload unencrytped file will abort - add unit test to verify the flag --force-unencrytped is removed
Replaced manual magic header ("crypt4gh") reading and file seeking across
upload and encrypt packages with the helpers.IsCrypt4GHFile utility function.
Add test cases covering valid Crypt4GH headers, unencrypted files, files under 8 bytes, and non-existent files
Use OS-aware filepath.Clean instead of path.Clean when opening local files in uploadFiles to ensure correct path handling across different operating systems.
Update TestUploadTargetDirFlagAfterFileName to verify that the uploaded object key actually includes the target directory prefix in S3, ensuring the flag is not silently ignored.
Remove os.Stderr redirection logic from TestUploadRecursive since no assertions are made on stderr output. This prevents swallowing runtime error logs during test failures.
Add a ListObjects assertion to TestUploadUnencryptedFileAborts to verify that no unencrypted file contents are written to S3 prior to aborting.
Update TestUploadRecursive to upload pre-encrypted Crypt4GH fixtures without the --encrypt-with-key flag.
ad76cec to
99e495a
Compare
Add checksum_*.md5 and checksum_*.sha256 to .gitignore to prevent leftover test files from cluttering the working directory if unit tests fail or panic prior to cleanup.
…ileName Update TestUploadTargetDirFlagAfterFileName to pass flags via os.Args so Cobra parses trailing flags after positional file arguments.
2 participants
Related issue(s) and PR(s)
This PR together with #720 closes #716
Description
New version 0.3.4 will be created after merging to main
How to test
--force-unencryptedrelated text--force-unencryptedrelated text