[RPM M1] Assemble artifacts based on distribution not file extension

[peterzhuamazon]

Signed-off-by: Peter Zhu [email protected]

Description

Hi,

This PR is to assemble artifacts based on distribution value in build manifest.
Instead of based on build workflow artifact file extension.
We will use distribution value in build manifest to assemble artifact.

This is the PR to follow up on #1629, as a pre-requisite to add RPM distribution extract/install/assemble.

Thanks.

Issues Resolved

Part of #1544

Check List

• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov-commenter]

Codecov Report

Merging #1659 (612c77a) into main (947bc5d) will decrease coverage by 0.22%.
The diff coverage is 96.15%.

@@             Coverage Diff              @@
##               main    #1659      +/-   ##
============================================
- Coverage     94.69%   94.46%   -0.23%     
  Complexity       14       14              
============================================
  Files           163      165       +2     
  Lines          3447     3471      +24     
  Branches         21       21              
============================================
+ Hits           3264     3279      +15     
- Misses          180      189       +9     
  Partials          3        3              

Impacted Files	Coverage Δ
src/manifests/build_manifest.py	100.00% <ø> (ø)
src/manifests/bundle_manifest.py	97.43% <75.00%> (-2.57%)	⬇️
src/assemble_workflow/bundle.py	94.73% <100.00%> (+0.14%)	⬆️
src/assemble_workflow/bundle_recorder.py	100.00% <100.00%> (ø)
src/assemble_workflow/dist.py	95.08% <100.00%> (-0.58%)	⬇️
src/assemble_workflow/dists.py	100.00% <100.00%> (ø)
src/system/temporary_directory.py	83.87% <0.00%> (-12.91%)	⬇️
src/system/os.py	92.30% <0.00%> (-7.70%)	⬇️
deployment/lib/artifacts-public-access.ts	100.00% <0.00%> (ø)
...loyment/lambdas/cf-url-rewriter/cf-url-rewriter.ts	50.00% <0.00%> (ø)
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 947bc5d...612c77a. Read the comment docs.

[peterzhuamazon]

@dblock need some help on the mypy error:

tests/tests_assemble_workflow/test_dist.py:49: error: "DistTar" has no attribute "_Dist__find_min_archive_path"  [attr-defined]
tests/tests_assemble_workflow/test_dist.py:56: error: "DistTar" has no attribute "_Dist__rename_archive_path"  [attr-defined]
tests/tests_assemble_workflow/test_dist.py:64: error: "DistTar" has no attribute "_Dist__rename_archive_path"  [attr-defined]

My test case is able to call the function and it exists in the code:

def test_find_min_archive_path(self) -> None:
        self.assertEqual(
            self.distTar._Dist__find_min_archive_path(self.artifacts_path),
            self.artifacts_path + "opensearch-1.3.0"
        )

The reason I call self.distTar._Dist__find_min_archive_path is because self.distTar.__find_min_archive_path will throw errors during the pytest.

If you have any experience on this or any advice, please let me know about it.

Thanks.

[peterzhuamazon]

@dblock need some help on the mypy error:

tests/tests_assemble_workflow/test_dist.py:49: error: "DistTar" has no attribute "_Dist__find_min_archive_path"  [attr-defined]
tests/tests_assemble_workflow/test_dist.py:56: error: "DistTar" has no attribute "_Dist__rename_archive_path"  [attr-defined]
tests/tests_assemble_workflow/test_dist.py:64: error: "DistTar" has no attribute "_Dist__rename_archive_path"  [attr-defined]

My test case is able to call the function and it exists in the code:

def test_find_min_archive_path(self) -> None:
        self.assertEqual(
            self.distTar._Dist__find_min_archive_path(self.artifacts_path),
            self.artifacts_path + "opensearch-1.3.0"
        )

The reason I call self.distTar._Dist__find_min_archive_path is because self.distTar.__find_min_archive_path will throw errors during the pytest.

If you have any experience on this or any advice, please let me know about it.

Thanks.

@peternied do you have any experience on this situation?
These functions exist as __<functionName>, if you call with __<functionName>, the test wont run but if you express them into _<ClassName>__<functionName> they will run, but then mypy complain about it.

Thanks.

Thanks.

[peternied]

What about making find_min_archive_path a non-private function, seems like that would resolve this problem?

It is anti-pythonic to create private methods as there is a notion that call code can be seen / used, see this SO post for additional opinions.

[peterzhuamazon]

What about making find_min_archive_path a non-private function, seems like that would resolve this problem?

It is anti-pythonic to create private methods as there is a notion that call code can be seen / used, see this SO post for additional opinions.

Will take a look on that, never thought about this aspect in Python.
Thanks.

[dblock]

LGTM! The comment above doesn't feel like a must have, but tests must pass to merge.