-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
feat: add Oracle support on Metricbeat Docker images #12890
feat: add Oracle support on Metricbeat Docker images #12890
Conversation
@sayden can you have a look at this? We should merge it. @kuisathaverat will fix the conflicts today. |
@kuisathaverat thanks for working on this, my friend! 🙂 I have taken a look at everything looks ok but we need a green CI before trying to merge. We have this issue
Which is reproducible if you do a |
I will move the https://github.com/elastic/beats/blob/21302f24583d278312dc05839436b06df7a776ef/.ci/packer_cache.sh file to another PR, it is the script used by Packer to make the pull image on the worker so you do not need to pull the image it is in the Jenkins worker. |
I've opened another PR only with the workers Docker cache script #13764, a couple of days after that PR will merge the Docker images will be in the Jenkins workers and this PR should work |
7b7bd67
to
26d241f
Compare
3a04a0e
to
b3de9c1
Compare
b3de9c1
to
1ef74a3
Compare
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.
LGTM
This seems to break travis builds, could we do something to avoid needing the oracle image in travis? Something like this can be also needed to run builds by community contributors. |
I do not think so since you have to sign a before to download the image, also we can not put a user and password in Travis, the only solution would be to disable the Oracle test on Travis |
I guess this would be fine, we will also have to find a way to have two kind of metricbeat images, one with Oracle and another one without it. |
I think that to have two Dockerfiles and change one for another by setting an environment variable (PROPIETARY_DOCKER_IMAGES=1) could be the solution |
we have changed the approach Oracle instantclient Basic 19 can be download without having to agree on a form, so we download the binaries and build the Docker image, the same for everybody. |
I will keep the |
@sayden Could you confirm that the new Docker image works for the test? |
jenkins, test this again please (there was an unrelated failure on kubernetes tests but I think it prevented to run the integration tests) |
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.
This is looking good.
I think we should remove the skips in the oracle tests:
=== RUN TestData
--- SKIP: TestData (0.00s)
metricset_test.go:20: Skip until a proper Docker image is setup for Metricbeat
PASS
ok github.com/elastic/beats/v7/x-pack/metricbeat/module/oracle/tablespace 0.012s
But this could be also changed in a follow up if @sayden is ok with this image.
I think I can remove the skips in this follow up #16833, so we can free @kuisathaverat I tested the new library, included in this image, and it worked ok (after un-skipping tests, of course 😄 ) |
depends on #13764