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

Add InCluster support as a CronJob #73

Closed
wants to merge 6 commits into from

Conversation

patricktalmeida
Copy link
Contributor

I liked this tool and wanted to fit one of my use cases, which was to send the output to Slack on a weekly basis.

This PR add a support for kor to run as a CronJob inside the cluster and send the output to Slack in 2 ways, either by sending raw text with a Slack webhook, or using hte files.upload Slack API to send the output as an attached file which supports formatted and bigger outputs than a webhook.

First of all, sorry about the big PR.
Let me know of your thinking on this.

If it satisfies and you want to have it merged, change the DockerHub repository from patricktoledodea/kor to one of yours on both release.yml and values.yaml as I won't be maintaining the image with all updates.

@yonahd
Copy link
Owner

yonahd commented Sep 12, 2023

Hey @patricktalmeida
This is an incredible contribution, great features most of which are on the roadmap.
It would be super helpful if we can break this down into multiple MR's.

  • Docker related stuff(I'll add credentials and a repository)
  • Slack related code changes
  • Helm chart(we would want to give the option to run this as a deployment with a loop in order to allow prometheus metrics)

@yonahd
Copy link
Owner

yonahd commented Sep 12, 2023

Also feel free to put your ideas here #69

@patricktalmeida
Copy link
Contributor Author

Hey @patricktalmeida This is an incredible contribution, great features most of which are on the roadmap. It would be super helpful if we can break this down into multiple MR's.

  • Docker related stuff(I'll add credentials and a repository)
  • Slack related code changes
  • Helm chart(we would want to give the option to run this as a deployment with a loop in order to allow prometheus metrics)

I'll break this into 3 branches as you asked and push them soon.

About the Deployment part, yeah a friend of mine suggested me to push those metrics to Prometheus in order to have warnings. I also like that feature, though I focused on the one that I really needed for now. We can discuss this one further.
The CronJob can also be left for whoever wants to use it as I left and enabled: true flag which people can opt in or out when the Deployment feature gets created.

Let me know if you have other comments meanwhile.

@yonahd
Copy link
Owner

yonahd commented Sep 12, 2023

Issue for docker ci #74

@yonahd
Copy link
Owner

yonahd commented Sep 12, 2023

The general direction is good. I want to add something like this #75

@patricktalmeida
Copy link
Contributor Author

patricktalmeida commented Sep 12, 2023

Yeah, this PR cover this #75, actually. I'll still split them into 3.
I changed a little the logic on GetKubeClient function and it also covers loading a SA with Role permissions.

@patricktalmeida
Copy link
Contributor Author

Closing this PR. This was split into: #76 #77 #78

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.

2 participants