-
Notifications
You must be signed in to change notification settings - Fork 183
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 makefile to vagrant to install collection, receiver-mock, grafana and supports avalanche #815
Conversation
vagrant/k8s/service-monitors.yaml
Outdated
- avalanche | ||
endpoints: | ||
- port: "http-avalanche" # Same as service's port name | ||
interval: 30s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same elsewhere: don't we want new line characters at the end of the file?
access_id = dummy | ||
access_key = dummy | ||
name = collection | ||
mkfile_path := $(abspath $(lastword $(MAKEFILE_LIST))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity why do we need this construct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted this Makefile to be executable, so need to get absolute path of the file and later abs path of the repository. I will add comments to clarify that
vagrant/README.md
Outdated
List of other useful commands in the Makefile: | ||
- expose-prometheus - exposes prometheus on port 9090 of virtual machine | ||
- expose-grafana - exposes grafana on port 8080 of virtual machine | ||
- apply-avalanche - run one pod deployment of avalanche (metrics generator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: don't we want to format the targets as code with backticks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor stuff. Overall this looks good to me.
vagrant/Makefile
Outdated
patch-context \ | ||
remove-tmp \ | ||
apply-namespace \ | ||
apply-receiver-mock \ | ||
create-secrets \ | ||
grafana-dashboards \ | ||
helm-dependencies \ | ||
helm-upgrade \ | ||
apply-service-monitors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a Makefile
we need tabs but why mix it with spaces here instead of just using 2 tabs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation in the terminal looks better with two spaces instead of eight. I tried to omit tabs but it's not possible 😞
@perk-sumo thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go with tabs to make it consistent across whole Makefile file.
vagrant/Makefile
Outdated
--namespace "${namespace}" \ | ||
--install \ | ||
--set sumologic.accessId="${access_id}" \ | ||
--set sumologic.accessKey="${access_key}" \ | ||
--set prometheus-operator.prometheus.prometheusSpec.externalLabels.cluster="${cluster}" \ | ||
--set sumologic.clusterName="${cluster}" \ | ||
--set sumologic.endpoint="${endpoint}" \ | ||
--set prometheus-operator.grafana.enabled=true \ | ||
-f "${root_dir}vagrant/values.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here with tabs/spaces and in a couple of other places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think those should be in external scripts at some point. No need to do it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for preparing this, it's a good start!
I bet this will evolve in time so even if I have some comments I think it's good to go :)
vagrant/k8s/avalanche.yaml
Outdated
spec: | ||
containers: | ||
- name: avalanche | ||
image: quay.io/freshtracks.io/avalanche:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather specify working version in here to omit any surprises.
vagrant/k8s/logs-generator.yaml
Outdated
spec: | ||
containers: | ||
- name: generator | ||
image: localhost:32000/sumologic/kubernetes-tools:local |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume local registry works by default and the image is there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't it be sumologic/kubernetes-tools:master
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file shouldn't be here 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the yaml comments guideline as discussed in #836 ?
Do we want to use this approach in other yaml files as well?
0781781
to
bb7d01b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I suppose those files are not going to be modified by the customer, so there is no need for that |
0caa020
to
f2c4b69
Compare
0b9fd87
to
142f139
Compare
142f139
to
7609b4d
Compare
Description
Add makefile to vagrant to install collection, receiver-mock, grafana and supports avalanche
Testing performed