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

Grafana sidecar for loading dynamically dashboards #40

Merged
merged 11 commits into from
Jun 26, 2020

Conversation

lzecca78
Copy link
Contributor

Hi everyone 👋 !
i've implemented the k8s-sidecar that allow everyone to eventually inject in grafana whatever dashboard just creating a configmap in his namespace and labelling it with the label-key defined in the deployment. (look at README.md for more details)
This make massively easier to add/remove/modify new/exisisting dashboard without blood and tears.
Looking forward for your review!

@lnovara
Copy link
Contributor

lnovara commented Jun 19, 2020

https://media.giphy.com/media/Qv5JV60U5Qm26Ipp9J/giphy.gif

@lnovara
Copy link
Contributor

lnovara commented Jun 19, 2020

Copy link
Contributor

@phisco phisco left a comment

Choose a reason for hiding this comment

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

LGTM, but please disable the on-save-linter because it makes me mad to see so many lines changed just for a blank space or a tab.

@lzecca78
Copy link
Contributor Author

@phisco the right way is the linter one. Once that all files will be linted, you'll never see any modifications 👨‍🔧

@phisco
Copy link
Contributor

phisco commented Jun 21, 2020

That’s the case if we all agree on a linter and/or spec, otherwise your linter will just keep removing spaces and white lines and mine (I don’t have one for yaml) could just keep adding them back 🤷🏻‍♂️ I love go fo having an official linter, that can be automated on save, yaml ones are just a matter of taste

@lzecca78
Copy link
Contributor Author

@phisco i guess that is worst going on without linter because at least, linter has a standard, no linter means that anyone can do whatever it likes, that means "mess".

@ralgozino
Copy link
Member

Now that the linting issue has been addressed and we've agreed on using police-man could you please add it to the pipeline @lzecca78 and let us know so we can move on with the review?
Thanks!

@lzecca78
Copy link
Contributor Author

@ralgozino the modification that you are requesting will impact all the module monitoring that is the grafana dir plus everything else. This is imho out of scope with the aim of this PR. If you want, we can create an issue that will be converted in an activity, but i guess that is totally unrelated with this one.

@ralgozino
Copy link
Member

as a minimum, I would say to apply police-man to the files that you are modifying in this PR, it is official since yesterday so we have to start enforcing it.
I don't know if it makes sense for the rest of the repo, I guess it depends on how many changes it will trigger, maybe the files are already mostly compliant with the linting rules.
Thoughts @angelbarrera92 @iknite @phisco ?

@angelbarrera92
Copy link
Contributor

I included policeman in a couple of modules and it requires some time to fix (possible) issues. But we are talking about.... 30 min? So i think is worthy to include it in the PR.

If you don't want, i can do it for you. Just let me know Luca

@lzecca78
Copy link
Contributor Author

Ok I will do it tomorrow!

@lzecca78
Copy link
Contributor Author

@angelbarrera92 @ralgozino , linting done

Copy link
Contributor

@angelbarrera92 angelbarrera92 left a comment

Choose a reason for hiding this comment

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

@angelbarrera92 @ralgozino , linting done

Awe some job luca. Just one more thing (sorry for it) but you have to add a dependency between pipelines, meaning that if policeman does not pass, no other pipelines are executed. You can see an example here: https://github.com/sighupio/fury-distribution/blob/master/.drone.yml#L18-L19

Also, make sure you use v0.5.0 version for quay.io/sighup/e2e-testing-drone-plugin container image. It contains our latest modification to add labels to the vsphere instances ;)

Thanks!

katalog/grafana/dashboards/kustomization.yaml Show resolved Hide resolved
katalog/grafana/kustomization.yaml Show resolved Hide resolved
katalog/grafana/rbac.yml Show resolved Hide resolved
@lzecca78
Copy link
Contributor Author

@angelbarrera92 added dependencies and modified image version in .drone.yml

@angelbarrera92 angelbarrera92 self-requested a review June 26, 2020 07:09
.drone.yml Outdated Show resolved Hide resolved
@angelbarrera92 angelbarrera92 self-requested a review June 26, 2020 08:19
Copy link
Contributor

@angelbarrera92 angelbarrera92 left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

6 participants