-
Notifications
You must be signed in to change notification settings - Fork 282
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
[Dashboards] integ-test in Jenkins #1653
[Dashboards] integ-test in Jenkins #1653
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1653 +/- ##
============================================
+ Coverage 94.68% 94.75% +0.06%
Complexity 14 14
============================================
Files 163 168 +5
Lines 3446 3506 +60
Branches 21 26 +5
============================================
+ Hits 3263 3322 +59
- Misses 180 181 +1
Partials 3 3
Continue to review full report at Codecov.
|
@@ -0,0 +1,132 @@ | |||
lib = library(identifier: "jenkins@20211118", retriever: legacySCM(scm)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to add this job to #1468 once PR is merged, so we can add test cases for this when libTesters
are ready
Would it be simpler and future-proof to read the agent from the test manifest just like we do from the build manifest? |
Will it change often? Regardless, I believe it would look cleaner and enables forks to have their own agents defined. |
8950881
to
cea6ac6
Compare
Thanks for your comments @abhinavGupta16, made the updates as suggested. |
@kavilla Thank you for making the changes. I have added a few more suggestions. |
@peterzhuamazon - We have a few stages that run without docker. Should we add docker to all the stages? |
a56608b
to
212330c
Compare
@opensearch-project/engineering-effectiveness, anyway to get more reviews on this PR? It would be a big win to have this running integration tests running for OpenSearch Dashboards. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why tests/jenkins/data/opensearch-1.3.0-build.yml
file is being added.
For testing purposes, the original jenkins/data files were added in here 6c54533 But to add it a test for runIntegTestScript it needed a build manifest but for the purpose of the test we added a build ID to a regular manifest. So it gave it my room to test on how manifests that go into the groovy script look like. I understand that there is https://github.com/opensearch-project/opensearch-build/tree/main/tests/data but given that more recent hash is in jenkins/data and the tests in the previous commit used jenkins/data it made sense to me to add the new file along with OpenSearch Dashboards. However, if asking how come OpenSearch and not just OpenSearch Dashboards this PR comes with inclusion of new tests. |
Execute the integ-test workflow for OpenSearch Dashboards including tests. As opposed to OpenSearch, we need an image with the appropriate libraries so we pass the image but it does not have the wget so the steps were broken to access the build manifest. Hardcoding the OpenSearch build id for now until this get resolves: opensearch-project#1492 ARM tests will fail still until this is resolved: opensearch-project#1381 Issue partially resolved: opensearch-project#704 Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
212330c
to
170d581
Compare
} | ||
stage('download-manifest') { | ||
agent { | ||
node { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should run this in a docker container
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Within OpenSearch integ-test, they are able to have a single step that downloads and runs the test. However, the image that is required to run the integration tests for OpenSearch Dashboards does not support wget
which is what the downloadBuildManifest
requires.
Would the suggestion be to hardcode this stage to utilize a docker image that would normally be used by OpenSearch?
Also for my own curiosity, what's the benefit of running within a docker container vs running within a node? Does this prevent potential conflicts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running within container ensures a fresh run every-time because the container will be destroyed after every run. Also, as you rightly mentioned, it also helps prevent conflicts. It would also ensure that the node is not affected by the job.
We can specify any image that would help support this operation.
Also, why are we taking image name as input from the user? Can these possibly change without changes to the job?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can specify any image that would help support this operation.
Do you know of which I can use? Used a couple docker images and all fail with line 1: wget: command not found
. It would appear that's why OpenSearch is doing the same for integ tests: https://github.com/opensearch-project/opensearch-build/blob/main/jenkins/opensearch/integ-test.jenkinsfile#L37
Also, why are we taking image name as input from the user?
When OpenSearch Dashboards builds it already detected the docker image and will require to use the appropriate image for example: https://github.com/opensearch-project/opensearch-build/blob/main/manifests/2.0.0/opensearch-dashboards-2.0.0.yml#L8. So when the build pipeline triggers the job it will pass the values it already has on hand.
Can these possibly change without changes to the job?
Apologies I'm not clear what you mean here could you clarify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can curl
instead of wget
I believe ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can
curl
instead ofwget
I believe ...
Secret sauce, was able to update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this job pick the docker image from the manifest file? If yes, we should not have an input param, but assign the image variable locally using manifest.
As of right now, the actual build manifest doesn't include the CI image stuff. The manifests in manifests/x.y.z/
do. There could be potential to pass MANIFEST
, TEST_MANIFEST
, and BUILD_MANIFEST_URL
. But if we are going that route I would rather we open an issue and address that in both OpenSearch and OpenSearch Dashboards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kavilla - In that case, can we code in the docker image rather than taking it as an input parameter? It would reduce one parameter for the user.
Once the manifest supports holding the image, the job would then parse the build manifest and get the image from there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could utilize #1687, which would have the image available. But that wouldn't resolve the issue about the number of params. I wouldn't see any issue if we did include the CI information into the actual build manifest to reduce the amount of params.
I created an issue for that: #1694 since that will require more refactoring within the actual build repo.
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
58779b4
to
2ca2fdb
Compare
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
@opensearch-project/engineering-effectiveness |
We are looking in to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont have too much issues with this PR except a few naming convention.
Thanks.
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Need @abhinavGupta16 to remove his block before we merge. |
Added comments. I believe we can get rid of the image name parameter. One less thing for the user to input and worry about |
I have approved it for now, however, we should look at resolving these new issues at the earliest. |
Description
Execute the integ-test workflow for OpenSearch Dashboards including
tests.
As opposed to OpenSearch, we need an image with the appropriate libraries
so we pass the image but it does not have the wget so the steps were broken
to access the build manifest.
Hardcoding the OpenSearch build id for now until this get resolves:
#1492
ARM tests will fail still until this is resolved:
#1381
Signed-off-by: Kawika Avilla kavilla414@gmail.com
Issues Partially Resolved
#704
Check List
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.