Add ci-group arg for OSD integ tests #567
Conversation
Signed-off-by: Rishabh Singh <[email protected]>
rishabh6788
force-pushed
the
main
branch
5 times, most recently
from
da8d27a to
f186299
Compare
rishabh6788
requested review from
peterzhuamazon,
prudhvigodithi,
gaiksaya,
Divyaasm and
zelinh
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #567 +/- ##
============================================
+ Coverage 84.75% 84.79% +0.04%
Complexity 114 114
============================================
Files 117 119 +2
Lines 682 684 +2
Branches 75 75
============================================
+ Hits 578 580 +2
Misses 30 30
Partials 74 74 ☔ View full report in Codecov by Sentry. |
| @@ -0,0 +1,54 @@ | |||
| /* | |||
There was a problem hiding this comment.
Not understanding why we need to add this library separately? What is the difference?
There was a problem hiding this comment.
It moves logic here https://github.com/opensearch-project/opensearch-build/blob/main/jenkins/opensearch-dashboards/integ-test.jenkinsfile#L251-L290 to avoid duplicating the same code in if-else block.
There was a problem hiding this comment.
I see. How about renaming it to runIntegTestScriptForOSD or runOSDintegTestScript?
Also do you recollect why we have that if/else block for windows? Sorry it being long since this was written.
cc: @peterzhuamazon
| steps { | ||
| script { | ||
| processAndRunOsdIntegTest( | ||
| localComponent: 'OpenSearch', |
There was a problem hiding this comment.
This should fail right? Since the lib as the name suggests only tests OSD?
There was a problem hiding this comment.
This is just a dummy component name to check component without any ci-group.
There was a problem hiding this comment.
Maybe pass dashboard component. Just a nit since we would never call this library for OS test manifest. Should we add a check in the lib making sure that test manifest belongs to OSD dist and not OS?
| sh("rm -rf ${WORKSPACE}/artifacts") | ||
| } | ||
| else { | ||
| echo "Not on Windows, unstash repository+artifacts" |
There was a problem hiding this comment.
Should this be
| echo "Not on Windows, unstash repository+artifacts" | |
| echo "Not on Windows, skipping unstashing repository+artifacts" |
I dont see any action being taken in else statement. Just the echo.
There was a problem hiding this comment.
If it is not a blocker we can revisit this later.
|
Can you also please add library definitions to these 2 libraries. Something similar to https://github.com/opensearch-project/opensearch-build-libraries/blob/main/vars/publishDistributionBuildResults.groovy#L10-L19 |
rishabh6788
force-pushed
the
main
branch
2 times, most recently
from
3e2eb6a to
a2ea550
Compare
vars/runIntegTestScriptForOSD.groovy
Outdated
| buildManifest: "${BUILD_MANIFEST}", | ||
| testManifest: "${TEST_MANIFEST}", | ||
| localPath: "${WORKSPACE}/${distribution}", |
There was a problem hiding this comment.
From where are we getting this values from? Are those args as well?
Same goes for ARTIFACT_BUCKET_NAME, artifactPathOpenSearch and artifactPath?
If yes can you add the same to lib description above?
There was a problem hiding this comment.
All being set as env variable in the jenkins job, and since the script is being executed in the same environment it has access to all the env variables.
There was a problem hiding this comment.
This sounds risky if someone needs to use the lib directly. It wont know what to specify. Those args are Parameters in the workflow thats why we can use it as ENV variable. I believe it we need to add as args and add checks.
There was a problem hiding this comment.
So runIntegTestScriptForOSD will have more args than usual one which is fine as it is reducing the complexity in main workflow.
Signed-off-by: Rishabh Singh <[email protected]>
Description
Add ci-group arg for OSD integ tests.
Issues Resolved
List any issues this PR will resolve, e.g. Closes [...].
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.