Skip to content
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

Dockerize es-indices-clean script #741

Merged

Conversation

pavolloffay
Copy link
Member

Resolves #740

Signed-off-by: Pavol Loffay ploffay@redhat.com

@@ -1,4 +1,4 @@
#!/bin/bash
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bash is not in alpine

@@ -1,6 +1,5 @@
import elasticsearch
import curator
import logging
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never used

@coveralls
Copy link

coveralls commented Mar 15, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 83c7a56 on pavolloffay:dockerize-clean-indices into 30d3e18 on jaegertracing:master.

@black-adder
Copy link
Contributor

For what purpose? Is usability improved by running the script inside the container?

@pavolloffay
Copy link
Member Author

How do you want to run it as cronjob without container? jaegertracing/jaeger-kubernetes#40

Copy link
Contributor

@isaachier isaachier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, LGTM. @black-adder isn't in the office this week. Maybe @vprithvi or @yurishkuro can give this a final look.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. the name of the Dockerhub repository might look better as (jaeger-)es-index-cleaner
  2. the es_indices_clean.sh looks like an anti-pattern. It will run pip install inside the container every time, I think we should only do this once at build time. And there's no need to have a shell script in the first place since we're already running in a python container - just execute python with the .py file directly

@pavolloffay
Copy link
Member Author

pavolloffay commented Mar 20, 2018

the name of the Dockerhub repository might look better as (jaeger-)es-index-cleaner

It uses the same naming convention as other images.

the es_indices_clean.sh looks like an anti-pattern.

Do you want to remove scrip altogether? I agree about pip.

@isaachier
Copy link
Contributor

The script might be useful for users not on Kubernetes, but they should probably use a real cronjob or Jenkins, so IMO makes sense to delete it.

@yurishkuro
Copy link
Member

the name of the Dockerhub repository might look better as (jaeger-)es-index-cleaner

It uses the same naming convention as other images.

There isn't really a convention. I I just think "es index cleaner" describes better what the image represents than "es indices clean".

The script might be useful for users not on Kubernetes, but they should probably use a real cronjob or Jenkins, so IMO makes sense to delete it.

The python script will still be available, I am just saying there's no reason for a shell script as well, especially because it does pip install, which is likely to fail depending on user permissions.

@pavolloffay
Copy link
Member Author

There isn't really a convention. I I just think "es index cleaner" describes better what the image represents than "es indices clean".

that yes, I didn't read it properly. I thought you want to remove jaeger prefix :/. I will rename it

@pavolloffay
Copy link
Member Author

@yurishkuro image has been renamed and shell script removed.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@pavolloffay pavolloffay merged commit 5c09726 into jaegertracing:master Mar 21, 2018
@ghost ghost removed the review label Mar 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants