feat(JA): Add force-rehash option to snapshot command. Integ test refactoring. #477
Conversation
leongdl
changed the title
| @@ -38,7 +40,9 @@ def _create_manifest_for_single_root( | |||
| total_input_files=upload_group.total_input_files, | |||
| total_input_bytes=upload_group.total_input_bytes, | |||
| print_function_callback=logger.echo, | |||
| hashing_progress_callback=hash_callback_manager.callback, | |||
| hashing_progress_callback=( | |||
There was a problem hiding this comment.
This was a BUG and printed out text for --json
There was a problem hiding this comment.
All unit tests w/ --json now does a json.loads of the output to make sure json parsing never breaks.
|
Could you remove the |
test/integ/cli/test_cli_manifest.py
Outdated
| # When | ||
| runner = CliRunner() | ||
| # Temporary, always add cli_manifest until launched. | ||
| main.add_command(cli_manifest) |
There was a problem hiding this comment.
nit - this is not needed anymore, we can probably do a pass to clean them all.
There was a problem hiding this comment.
Good call, let me scrub them.
| @@ -213,6 +213,15 @@ def _manifest_diff( | |||
|
|
|||
| output: ManifestDiff = ManifestDiff() | |||
|
|
|||
| # Helper function to update output datastructure. | |||
| def process_output(status: FileStatus, path: str, output_diff: ManifestDiff): | |||
There was a problem hiding this comment.
This file is a public interface, we should keep this helper private by prefixing _process_output.
There was a problem hiding this comment.
No need, this is a nested function in a function! So its not exposed.
Line 178 above already has _manifest_diff
There was a problem hiding this comment.
Ahh didn't notice it's nested. Nice!
| @pytest.mark.skip("Random Failure with no credentials on Github") | ||
| class TestSnapshot: |
There was a problem hiding this comment.
Curious, what was the reason and what has been done to avoid the random failures?
There was a problem hiding this comment.
This file was moved from unit tests. In Unit tests, the mocks changed the AWS creds, and that caused a very weird behavior depending on how unit tests were scheduled. This caused the credentials to be suddenly unmocked!
leongdl
changed the title
…s for Diff and Snapshot to Integ Signed-off-by: David Leong <[email protected]>
Doh, this was my test file for snapshot. I snapshot the source code itself. |
|
What was the problem/requirement? (What/Why)
What was the solution? (How)
manifest snapshot. Inmanifest diff, I added aforce-rehashoption code path that allows users to rehash all files. I'm adding this also to snapshot so users can explicitly re-hash all files instead of comparing only by time stamp.What is the impact of this change?
How was this change tested?
hatch run testpasses.hatch run integ:testpasses.downloadorasset_syncmodules? If so, then it is highly recommendedthat you ensure that the docker-based unit tests pass.
Was this change documented?
Are relevant docstrings in the code base updated?
Has the README.md been updated? If you modified CLI arguments, for instance.
Does this PR introduce new dependencies?
This library is designed to be integrated into third-party applications that have bespoke and customized deployment environments. Adding dependencies will increase the chance of library version conflicts and incompatabilities. Please evaluate the addition of new dependencies. See the Dependencies section of DEVELOPMENT.md for more details.
Is this a breaking change?
Does this change impact security?
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.