-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, fixed in latest push
if chunk != b'': | ||
chunk = True | ||
while chunk: | ||
chunk = target_file.read(100000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curious why 100 000 bytes specifically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this function used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
@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.