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
Services / Replicas #51
Conversation
- adding the ability to controller the labels and annotations on the service and ingress which is useful for addon components or clouds specifics (cert-manaer, gke ingress, eks etc)
@gambol99 That looks awesome, thanks for the contribution! I'd like to get this reviewed and tested as soon as possible. I'll see if i get a chance next week.
I think this would deprecate the |
common.GrafanaDatasourcesConfigMapName, | ||
common.GrafanaServiceName, | ||
} | ||
if cr.Spec.Hostname != "" { |
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.
Why only create the Ingress/Route when a Hostname is set? Its not a requirement for OpenShift Routes (gets assigned automatically if not provided) and i believe in vanilla Kubernetes this is valid too.
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.
hi @pb82 ... I guess the thinking was to provide a means to control if you wanted a ingress, or defer to a LoadBalancer svc. I was gonna fold this into a spec.ingress.enabled option but wanted to keep it compatible with previous versions. If you prefer though, I'll drop the change and keep the ingress as is?
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.
Making the Ingress/Route optional sounds ok to me. So is your idea to replace this with the ingressEnabled
flag? What do you think about providing a spec.ingressSettings
struct with two options: enabled
and hostname
? This would be used for routes as well and the default option is enabled with no hostname set.
At some point we would have to deprecate the old hostname option, but i can take care of that.
imagePullPolicy: IfNotPresent | ||
name: grafana | ||
env: | ||
- name: CONFIG_HASH |
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 like this, much better solution for restarts 👍
- Switching to using a map[string]string instead of the metav1.LabelSelector as it's makes more sense
@gambol99 I think this looks good overall. I've created a PR against your branch that updates the docs, CRDs and adds |
hi @pb82 .. sorry for the delay.. i've merged in your branch in .. :-) |
@gambol99 Awesome, thanks for the great work! I've merged your changes and will soon release them as part of v2.0.0 after merging some other open PRs! |
* Document way of working * e2e workflow * install kuttl through makefile
* Document way of working * e2e workflow * install kuttl through makefile
* Document way of working * e2e workflow * install kuttl through makefile
Apologizes for bundling these in a single PR I was making changes, deploying and testing at the same time. I've separated changes into individual commits, but if you prefer and try make each a PR