Skip to content
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

Support multiple values in --component. #2110

Merged
merged 3 commits into from
May 12, 2022

Conversation

dblock
Copy link
Member

@dblock dblock commented May 11, 2022

Signed-off-by: dblock dblock@amazon.com

Description

Added support for multiple values in --component in various build, test and sign workflows.

I chose to support --component x y vs. --component x --component y to make it easier to specify from Jenkins. In code that's done with nargs='+' vs. action='append', see https://stackoverflow.com/questions/15753701/how-can-i-pass-a-list-as-a-command-line-argument-with-argparse.

Together with #2100 this change allows on-demand rebuilding of a subset of components in a manifest when it contains a subsequent component that has a broken build. This is useful when wanting to increment versions and needing a new -SNAPSHOT build of OpenSearch or common-utils for dependencies that are already in a manifest. Avoids having to pull out a component from the manifest and then reverting that change every time.

Issues Resolved

#2091

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.

@dblock dblock requested a review from a team as a code owner May 11, 2022 17:29
@dblock dblock requested a review from peterzhuamazon May 11, 2022 17:29
@codecov-commenter
Copy link

codecov-commenter commented May 11, 2022

Codecov Report

Merging #2110 (ca7441d) into main (f318149) will decrease coverage by 0.02%.
The diff coverage is 96.25%.

@@             Coverage Diff              @@
##               main    #2110      +/-   ##
============================================
- Coverage     94.24%   94.21%   -0.03%     
  Complexity       25       25              
============================================
  Files           203      204       +1     
  Lines          3907     3943      +36     
  Branches         29       29              
============================================
+ Hits           3682     3715      +33     
- Misses          219      222       +3     
  Partials          6        6              
Impacted Files Coverage Δ
src/manifests/component_manifest.py 92.00% <83.33%> (-3.24%) ⬇️
src/manifests/input_manifest.py 96.55% <92.30%> (-0.68%) ⬇️
src/build_workflow/build_args.py 100.00% <100.00%> (ø)
src/ci_workflow/ci_args.py 100.00% <100.00%> (ø)
src/ci_workflow/ci_input_manifest.py 100.00% <100.00%> (ø)
src/run_build.py 91.11% <100.00%> (ø)
src/run_sign.py 91.66% <100.00%> (-3.79%) ⬇️
src/sign_workflow/sign_args.py 100.00% <100.00%> (ø)
src/sign_workflow/sign_artifacts.py 98.07% <100.00%> (+0.03%) ⬆️
src/test_workflow/bwc_test/bwc_test_runner.py 92.30% <100.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 f318149...ca7441d. Read the comment docs.

@peterzhuamazon
Copy link
Member

@dblock

================================== FAILURES ===================================
________________ TestBuildManifest.test_select_two_are_unknown ________________

self = <tests.tests_manifests.test_build_manifest.TestBuildManifest testMethod=test_select_two_are_unknown>

    def test_select_two_are_unknown(self) -> None:
        path = os.path.join(self.data_path, "build", "opensearch-build-schema-version-1.2.yml")
        manifest = BuildManifest.from_path(path)
        with self.assertRaises(ValueError) as ctx:
            self.assertEqual(len(list(manifest.components.select(focus=["x", "y"]))), 0)
>       self.assertEqual(str(ctx.exception), "Unknown components=y,x.")
E       AssertionError: 'Unknown components=x,y.' != 'Unknown components=y,x.'
E       - Unknown components=x,y.
E       ?                     --
E       + Unknown components=y,x.
E       ?                    ++

tests\tests_manifests\test_build_manifest.py:113: AssertionError

@dblock dblock force-pushed the component-multiple branch from df35ee3 to fad4f60 Compare May 11, 2022 19:02
@dblock
Copy link
Member Author

dblock commented May 11, 2022

@dblock

================================== FAILURES ===================================
________________ TestBuildManifest.test_select_two_are_unknown ________________

self = <tests.tests_manifests.test_build_manifest.TestBuildManifest testMethod=test_select_two_are_unknown>

    def test_select_two_are_unknown(self) -> None:
        path = os.path.join(self.data_path, "build", "opensearch-build-schema-version-1.2.yml")
        manifest = BuildManifest.from_path(path)
        with self.assertRaises(ValueError) as ctx:
            self.assertEqual(len(list(manifest.components.select(focus=["x", "y"]))), 0)
>       self.assertEqual(str(ctx.exception), "Unknown components=y,x.")
E       AssertionError: 'Unknown components=x,y.' != 'Unknown components=y,x.'
E       - Unknown components=x,y.
E       ?                     --
E       + Unknown components=y,x.
E       ?                    ++

tests\tests_manifests\test_build_manifest.py:113: AssertionError

Fixed. Subtracting sets did not necessarily preserve order, so I fixed it with invalid = [item for item in focus if item not in self] vs. invalid = focus - self.keys().

Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dblock I do have some questions on several parts.
Thanks.

@peterzhuamazon
Copy link
Member

Also @dblock please update the jenkins file of both distribution builds params description, so user can take multiple components with spaces.
Thanks.

Signed-off-by: dblock <dblock@amazon.com>
@dblock dblock force-pushed the component-multiple branch from fad4f60 to be164b1 Compare May 11, 2022 21:18
@dblock
Copy link
Member Author

dblock commented May 11, 2022

Also @dblock please update the jenkins file of both distribution builds params description, so user can take multiple components with spaces. Thanks.

Done. I didn't change COMPONENT_NAME to COMPONENT_NAMES or anything, but lmk if you think we should. I didn't want to break backwards compatibility in build.sh either and left it as --component.

@dblock dblock requested a review from peterzhuamazon May 11, 2022 21:21
Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some missing parts.

Signed-off-by: dblock <dblock@amazon.com>
@dblock dblock force-pushed the component-multiple branch from ca7441d to f09daf9 Compare May 11, 2022 23:27
@dblock dblock requested a review from peterzhuamazon May 11, 2022 23:28
Signed-off-by: dblock <dblock@amazon.com>
@peterzhuamazon
Copy link
Member

Thanks @dblock for this quick PR will use this to build 2.0.0.
Thanks.

@peterzhuamazon peterzhuamazon merged commit 28d5c7b into opensearch-project:main May 12, 2022
@dblock dblock deleted the component-multiple branch May 12, 2022 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants