Use es-index-cleaner golang implementation #3204
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3204 +/- ##
=======================================
Coverage 95.98% 95.99%
=======================================
Files 242 242
Lines 14794 14789 -5
=======================================
- Hits 14200 14196 -4
Misses 514 514
+ Partials 80 79 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
LGTM, just want to hear from @Ashmita152 on the Dockerfile changes.
| @@ -0,0 +1,6 @@ | |||
| ARG base_image | |||
There was a problem hiding this comment.
@Ashmita152, are you interested in reviewing this?
pavolloffay
force-pushed
the
es-cleaner-docker
branch
from
d060a71
to
a1551dc
Compare
|
The index cleaer integration test is failing bc it cannot build the image via buildx. The buildx uses different CI runner. |
|
@pavolloffay I think you would need to start registry container in elasticsearch integration tests i.e. add this section https://github.com/jaegertracing/jaeger/blob/master/.github/workflows/ci-hotrod.yml#L12-L16 in https://github.com/jaegertracing/jaeger/blob/master/.github/workflows/ci-elasticsearch.yml for fixing the CI failures. |
Makefile
Outdated
|
|
||
| .PHONY: index-cleaner-integration-test | ||
| index-cleaner-integration-test: docker-images-elastic | ||
| index-cleaner-integration-test: | ||
| $(MAKE) BASE_IMAGE=alpine:3.13 docker-images-elastic |
There was a problem hiding this comment.
Curious why we are overwriting BASE_IMAGE here ? I would like to keep a single source of truth for all base images and debug images.
There was a problem hiding this comment.
I don't like that building the images requires running docker registry at localhost. This complicates local and CI setup.
There was a problem hiding this comment.
I think reverting this change back should fix the current CI failure.
Here it is trying to tag the image name as alpine:3.13
make BASE_IMAGE=alpine:3.13 docker-images-elastic
make[1]: Entering directory '/home/runner/work/jaeger/jaeger'
docker buildx build -t alpine:3.13 --push \
--build-arg root_image=alpine:3.13 \
--build-arg cert_image=alpine:3.13 \
--platform=linux/amd64,linux/s390x,linux/ppc64le,linux/arm64 \
docker/base
which it later tries to push in the official alpine image resulting in
error: failed to solve: authorization status: 401: authorization failed
There was a problem hiding this comment.
yes this was the problem. I will try to play with tit and see if I could run it locally.
Can you run the full build locally? I had some issues with running the registry locally.
There was a problem hiding this comment.
@pavolloffay I had issues with local builds as well in the past; I managed to get it going I think. Maybe this helps? https://github.com/jaegertracing/jaeger/pull/2948/files#r666710826
|
the CI is all green now |
pavolloffay
force-pushed
the
es-cleaner-docker
branch
from
e7ceb25
to
280fb28
Compare
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
pavolloffay
force-pushed
the
es-cleaner-docker
branch
from
280fb28
to
248359d
Compare
|
PR rebased and ready for review/merge. |
Makefile
Outdated
| @@ -316,10 +320,13 @@ docker-images-cassandra: | |||
| docker build -t $(DOCKER_NAMESPACE)/jaeger-cassandra-schema:${DOCKER_TAG} plugin/storage/cassandra/ | |||
| @echo "Finished building jaeger-cassandra-schema ==============" | |||
|
|
|||
| docker-images-elastic: TARGET = release | |||
There was a problem hiding this comment.
I thought --target would be useful if a Dockerfile contained multiple build stages like debug and release. However, from what I see, there is only a release build stage in cmd/es-index-cleaner/Dockerfile. Would it be useful to still include the target param in this case?
There was a problem hiding this comment.
True, this can be removed. Done.
Signed-off-by: Pavol Loffay p.loffay@gmail.com
Updates #3179