Fix file truncation in Service Fabric commands #3796
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3796 +/- ##
==========================================
+ Coverage 70.95% 71.14% +0.18%
==========================================
Files 429 429
Lines 27154 27160 +6
Branches 4136 4137 +1
==========================================
+ Hits 19268 19322 +54
+ Misses 6623 6572 -51
- Partials 1263 1266 +3
Continue to review full report at Codecov.
|
|
Please rebase and move your test files and recording files. The location of those files was moved in |
| if os.path.isdir(abspath): | ||
| return abspath | ||
| else: | ||
| raise CLIError("Invalid path to application directory: {0}".format(abspath)) |
There was a problem hiding this comment.
I would suggest a ValueError to be throw instead. The error here is not CLI specific but input parameter issue.
There was a problem hiding this comment.
Agreed, fixed in latest push
| if chunk != b'': | ||
| chunk = True | ||
| while chunk: | ||
| chunk = target_file.read(100000) |
There was a problem hiding this comment.
Out of curious why 100 000 bytes specifically?
There was a problem hiding this comment.
It's the amount we want to read before we print the next update saying we've read more of the file. For when a user specifies --show-progress
| @@ -55,6 +56,38 @@ def app_type_returns_none_test(self, mock_config_parser): | |||
|
|
|||
| self.cmd("az sf application type", checks=[NoneCheck()]) | |||
|
|
|||
| def valid_application_upload_path_returns_absolute_path_test(self): | |||
There was a problem hiding this comment.
Is this function used anywhere?
There was a problem hiding this comment.
It's a test function, did you mean a different line?
|
|
||
| # Specifically need a large temporary file in a temporary directory | ||
| temp_app_dir = tempfile.mkdtemp() | ||
| dest_app_dir = os.path.join(os.path.dirname(temp_app_dir), "tempdir00") |
There was a problem hiding this comment.
Why not just create a folder under the current working directory? The logic here doesn't benefit from using tempfile.mkdtemp()
There was a problem hiding this comment.
I wasn't sure if we could use cwd as a valid place to generate a file, hence the use of tempfile. I thought it would be a safer place to generate the directory.
| fd, file_path = tempfile.mkstemp(dir=dest_app_dir) | ||
| os.close(fd) | ||
| dest_file_path = os.path.join(dest_app_dir, "tempfile00") | ||
| os.rename(file_path, dest_file_path) |
|
@troydai Is it possible to get this added to the July 6 milestone? |
|
It's in. |
Fixes #3666 and adds automated testing to cover this case.