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

Add Windows Min Artifacts Building on Jenkins #2483

Conversation

peterzhuamazon
Copy link
Member

Description

Add Windows Min Artifacts Building on Jenkins

Issues Resolved

#2306

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.

Signed-off-by: Peter Zhu <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Aug 19, 2022

Codecov Report

Merging #2483 (2bb5633) into main (e662f51) will increase coverage by 5.36%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##               main     #2483      +/-   ##
=============================================
+ Coverage     94.63%   100.00%   +5.36%     
=============================================
  Files           213         6     -207     
  Lines          4324       105    -4219     
  Branches         29        19      -10     
=============================================
- Hits           4092       105    -3987     
+ Misses          226         0     -226     
+ Partials          6         0       -6     
Impacted Files Coverage Δ
src/system/temporary_directory.py
...est_workflow/bwc_test/bwc_test_start_properties.py
src/test_workflow/integ_test/integ_test_runner.py
src/run_build.py
..._workflow/opensearch/build_artifact_check_maven.py
src/assemble_workflow/bundle_opensearch.py
src/assemble_workflow/bundle.py
tests/jenkins/jobs/Hello_Jenkinsfile
...est_workflow/test_result/test_component_results.py
src/jenkins/BuildManifest.groovy
... and 197 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This is fine.

I think we should raise the bar on the retry implementation and take a separate function from https://stackoverflow.com/questions/42521549/retry-function-in-python for example. Put it into system.... and write a test for it.

src/system/temporary_directory.py Outdated Show resolved Hide resolved
src/system/temporary_directory.py Outdated Show resolved Hide resolved
src/system/temporary_directory.py Outdated Show resolved Hide resolved
src/system/temporary_directory.py Outdated Show resolved Hide resolved
@peterzhuamazon
Copy link
Member Author

Hi @dblock I agree with many points you raised but not agree with putting the retries into another function.
The func(path) is specifically to onerror and requires a specific path param.
If we want to make a function that is generalized for everything we need to write complicated code just to take x number of params from different functions which is not very suitable.

Signed-off-by: Peter Zhu <[email protected]>
@peterzhuamazon peterzhuamazon merged commit 7b61ab4 into opensearch-project:main Aug 24, 2022
@peterzhuamazon peterzhuamazon deleted the opensearch-windows-bundle branch August 24, 2022 18:38
@peterzhuamazon peterzhuamazon restored the opensearch-windows-bundle branch September 1, 2022 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New Enhancement windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants