-
Notifications
You must be signed in to change notification settings - Fork 990
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 integration tests for Elasticsearch meter registry #1434
Conversation
It didn't happen in my local builds and looking at the commit (0e4343b) which the test started to fail in this branch, it doesn't seem to be relevant to this change. It might be related to some dependency update via dynamic versioning. |
I rebased this onto the current |
@shakuzen I rebased this onto master, and removed the blocking |
bad783d
to
4ffea53
Compare
I'm pretty sure I found the way to get Docker stuff working in CircleCI when I set CI things up for https://github.com/micrometer-metrics/prometheus-rsocket-proxy/. I meant to come back and apply it here so we could get this merged and tested. That said, I've also been considering switching the CI to GitHub Actions, but that probably wouldn't happen before the 1.4 release which I'm hoping to get out by the end of the year. I'll try to take a look at this more once I get some other work for 1.4 done first. |
FTR the easiest way to get Docker on CircleCI is to use the machine executor: machine:
enabled: true See Testcontainers' own CircleCI config: |
@bsideup @shakuzen seems to be looking for another approach to avoid build time increase. See #1434 (comment) |
haven't looked at the code, but the docker ci ran the docker tests and I verified the other didn't
|
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.
bumping versions as better now than a surprise break tomorrow
...test/java/io/micrometer/elastic/ElasticsearchMeterRegistryElasticsearch7IntegrationTest.java
Outdated
Show resolved
Hide resolved
...test/java/io/micrometer/elastic/ElasticsearchMeterRegistryElasticsearch6IntegrationTest.java
Outdated
Show resolved
Hide resolved
...test/java/io/micrometer/elastic/ElasticsearchMeterRegistryElasticsearch5IntegrationTest.java
Outdated
Show resolved
Hide resolved
Co-Authored-By: Adrian Cole <adriancole@users.noreply.github.com>
Co-Authored-By: Adrian Cole <adriancole@users.noreply.github.com>
Co-Authored-By: Adrian Cole <adriancole@users.noreply.github.com>
@adriancole Thanks for the feedback! I applied your suggested changes. |
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.
Looks good. Just a couple comments.
...c/src/test/java/io/micrometer/elastic/AbstractElasticsearchMeterRegistryIntegrationTest.java
Outdated
Show resolved
Hide resolved
...c/src/test/java/io/micrometer/elastic/AbstractElasticsearchMeterRegistryIntegrationTest.java
Outdated
Show resolved
Hide resolved
Co-Authored-By: Tommy Ludwig <8924140+shakuzen@users.noreply.github.com>
@shakuzen Thanks for the feedback! I applied your suggested changes. |
This PR adds integration tests on Elasticsearch meter registry for Elasticsearch 5 and 6. We can add one for Elasticsearch 7 once #1428 has been merged.
Closes gh-1429