azure-sdk-for-python icon indicating copy to clipboard operation
azure-sdk-for-python copied to clipboard
verified publisher icon Azure

Refactor logic for pre-processing local upload paths, address symlink bugs [DO NOT MERGE]

Open diondrapeck opened this issue 3 years ago • 3 comments

Description

Please add an informative description that covers that changes made by the pull request and link all relevant issues.

  • Refactor logic for pre-processing local upload paths and constructing remote paths for clarity
  • Address symlink bugs including https://github.com/Azure/azure-sdk-for-python/issues/27848

If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.

All SDK Contribution checklist:

  • [ ] The pull request does not introduce [breaking changes]
  • [ ] CHANGELOG is updated for new features, bug fixes or other significant changes.
  • [ ] I have read the contribution guidelines.

General Guidelines and Best Practices

  • [x] Title of the pull request is clear and informative.
  • [x] There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • [x] Pull request includes test coverage for the included changes.

diondrapeck avatar Dec 15 '22 02:12 diondrapeck

API change check

API changes are not detected in this pull request.

azure-sdk avatar Dec 15 '22 03:12 azure-sdk

@lalala123123 I don't have context on _additional_includes.py. This PR changes the way we obtain the list of files to be uploaded to include previously missing symlink categories. That would replace traverse_directory - it causes no issues in any other file or tests, but it does cause _additional_includes tests in test_component.py to fail.

1) Do you have any idea why and 2) Is it imperative that _additional_includes' methods use the same file retrieval method as we use for normal datastore uploads?

Thanks!

diondrapeck avatar Dec 16 '22 19:12 diondrapeck

@lalala123123 I don't have context on _additional_includes.py. This PR changes the way we obtain the list of files to be uploaded to include previously missing symlink categories. That would replace traverse_directory - it causes no issues in any other file or tests, but it does cause _additional_includes tests in test_component.py.

1) Do you have any idea why and 2) Is it imperative that _additional_includes' methods use the same file retrieval method as we use for normal datastore uploads?

Thanks!

@lalala123123 bumping this

diondrapeck avatar Dec 27 '22 17:12 diondrapeck

@lalala123123 I don't have context on _additional_includes.py. This PR changes the way we obtain the list of files to be uploaded to include previously missing symlink categories. That would replace traverse_directory - it causes no issues in any other file or tests, but it does cause _additional_includes tests in test_component.py to fail.

1) Do you have any idea why and 2) Is it imperative that _additional_includes' methods use the same file retrieval method as we use for normal datastore uploads?

Thanks!

  1. offline sync with Diondra
  2. _additional_includes calls this method to list out the src file and filter out the ignore file

lalala123123 avatar Jan 10 '23 15:01 lalala123123

@diondrapeck Hi, this PR has broken additional includes by adding an extra parent for files in additional includes: hello.py => hello.py/hello.py

I have tracked the bug with Bug 2193060: hello.py => hello.py/hello.py in additional includes. Please add Zhengfei & me as reviewer next time when you modify additional includes, thanks!

elliotzh avatar Jan 28 '23 07:01 elliotzh

@elliotzh thanks for the note. Can you add a test for this path so we have coverage?

paulshealy1 avatar Feb 02 '23 16:02 paulshealy1