-
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
fix: use supported-versions variables in ceph-api #23602
Conversation
@@ -13,11 +13,11 @@ services: | |||
- 8003 | |||
- 8080 | |||
ceph-api: | |||
image: docker.elastic.co/integrations-ci/beats-ceph:master-6373c6a-jewel-centos-7-x86_64-1 | |||
image: docker.elastic.co/integrations-ci/beats-ceph:${CEPH_VERSION:-master-97985eb-nautilus-centos-7-x86_64}-1 |
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'm not sure why this image is version -1
and the regular ceph is -2
. @jsoriano could you please 👀 here?
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 think we wanted to keep using an specific version here, that is different to the one in the ceph
service.
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
@@ -13,11 +13,11 @@ services: | |||
- 8003 | |||
- 8080 | |||
ceph-api: | |||
image: docker.elastic.co/integrations-ci/beats-ceph:master-6373c6a-jewel-centos-7-x86_64-1 | |||
image: docker.elastic.co/integrations-ci/beats-ceph:${CEPH_VERSION:-master-97985eb-nautilus-centos-7-x86_64}-1 |
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 don't think we can use the same versions in both cases, if I remember well, we kept this here to keep some tests specifically running against some specific older version.
I was wondering how is it possible then that tests pass after the changes in this PR, and I have found that there isn't actually anything using at the moment the ceph-api
service. It is not used in the python system tests, and go integration tests are skipped as flaky: #22612
Changes here probably need to be deeper, and maybe not only on the testing side. I think that we need to fix the integration tests and check what system tests are actually testing. Maybe at the end we can use a single ceph
service, but we need to make E2E and go integration tests smarter so they know what metricsets they should be using for each versions. This is what the system tests seem to be doing.
Or maybe we can refactor the metricsets so they detect the version and do nothing if they implement collection methods that are not supported. This way we could enable all metricsets with any version. But I'd say that changes in this module have low priority at the moment.
@@ -13,11 +13,11 @@ services: | |||
- 8003 | |||
- 8080 | |||
ceph-api: | |||
image: docker.elastic.co/integrations-ci/beats-ceph:master-6373c6a-jewel-centos-7-x86_64-1 | |||
image: docker.elastic.co/integrations-ci/beats-ceph:${CEPH_VERSION:-master-97985eb-nautilus-centos-7-x86_64}-1 |
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 think we wanted to keep using an specific version here, that is different to the one in the ceph
service.
This pull request does not have a backport label. Could you fix it @mdelapenya? 🙏
NOTE: |
PRs grooming - Closing for now until further updates. |
What does this PR do?
It uses the supported-versions env variables for the ceph-api service in the docker-compose.yml file for the integration.
Why is it important?
It seems values are hardcoded, and that could be affecting the E2E tests.
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.How to test this PR locally
Related issues