Skip to content
This repository has been archived by the owner on Jan 24, 2023. It is now read-only.

Chart dependencies #15

Closed
wants to merge 11 commits into from
Closed

Chart dependencies #15

wants to merge 11 commits into from

Conversation

valeriano-manassero
Copy link
Contributor

Original PR on Trains chart was referred here:
#12

@valeriano-manassero
Copy link
Contributor Author

also fixed a typo in ci checks

@sapir-allegro
Copy link
Contributor

Hey @valeriano-manassero,
I see that the versions you used for Redis, MongoDB and Elasticsearch are not the same versions we use in ClearML.
Any specific reason? Can you change those to the versions we use in ClearML?

@valeriano-manassero
Copy link
Contributor Author

Hey @valeriano-manassero,
I see that the versions you used for Redis, MongoDB and Elasticsearch are not the same versions we use in ClearML.
Any specific reason? Can you change those to the versions we use in ClearML?

I just tested latest stable on my testing cluster and they where working. I don't think there is any problem do downgrade to already used version. Will test the chart and update PR asap!

@valeriano-manassero
Copy link
Contributor Author

Hey @sapir-allegro , I updated dependencies versions to get in sync with the ones previously used.

Since I'm now using my fork to deploy ClearML I tested this PR very quickly and looks good to me but I suggest to install on one of your cluster to do some intensive tests.

Jfi even newest versions of mongo,redis and elastic works from what I can see.

Let me know if it's ok now.

@sapir-allegro
Copy link
Contributor

Hey @valeriano-manassero,
Thanks! this looks great.
I was also wondering about the env variables that were removed.
We've had some elastic properties defined using env variables and I see that you removed all of them.
Can you please add those back?

Regarding the newer versions - I'm happy to hear that it works! But we still need to test those before making such a change and it would probably take some time. I prefer using the current versions so we can merge this PR asap :)

@valeriano-manassero
Copy link
Contributor Author

Hey @sapir-allegro , I put back env vars as requested. Just too be extremely sure, since this is a big PR, I suggest to test a complete installation on your environments. As I said before, I tried this chart on my env and lgtm but better play safe 😄

@sapir-allegro
Copy link
Contributor

Great thanks!
And no worries we will test it before merging.

@jkhenning
Copy link
Member

@valeriano-manassero can we close this PR now that it was actually adopted as the clearml-server-cloud-ready chart? 🙂

@valeriano-manassero
Copy link
Contributor Author

Absolutely, just did it 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants