-
Notifications
You must be signed in to change notification settings - Fork 47
Conversation
46a226b
to
ca844ca
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.
Perhaps we could add some tests.
@@ -47,6 +47,12 @@ component "prometheus-operator" { | |||
node_selector = { | |||
"kubernetes.io/hostname" = "worker3" | |||
} | |||
tolerations { |
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 wonder if we could somehow improve this experience for users. E.g. being able to identify what workloads are part of the component and then do some smart mapping to worker groups, so you don't have to specify those manually.
ca844ca
to
06e4198
Compare
06e4198
to
d54d321
Compare
@invidian do we want package tests or e2e test? |
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.
@invidian do we want package tests or e2e test?
I think having at least manifest rendering tests touching all new options would be good.
d54d321
to
e6d7dec
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.
Just one more nit, otherwise looks good!
I suggest adding: diff --git a/pkg/components/prometheus-operator/component_test.go b/pkg/components/prometheus-operator/component_test.go
index 93c2911a..c161b7d9 100644
--- a/pkg/components/prometheus-operator/component_test.go
+++ b/pkg/components/prometheus-operator/component_test.go
@@ -24,6 +24,8 @@ import (
//nolint:funlen
func TestRenderManifest(t *testing.T) {
+ t.Parallel()
+
tests := []struct {
desc string
hcl string
@@ -143,6 +145,8 @@ component "prometheus-operator" {
for _, tc := range tests {
tc := tc
t.Run(tc.desc, func(t *testing.T) {
+ t.Parallel()
+
b, d := util.GetComponentBody(tc.hcl, Name)
if d != nil {
t.Fatalf("error getting component body: %v", d)
@@ -177,6 +181,8 @@ component "prometheus-operator" {
//nolint:funlen
func TestConversion(t *testing.T) {
+ t.Parallel()
+
testCases := []struct {
name string
inputConfig string This cuts down test execution time from 14 seconds to 4 seconds on my machine. Still slow, but better. |
ae4caed
to
9fa1767
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.
Looks OK
I only commented, not requested changes :) Changes looks good to me, but I think some other maintainer should have a look into it and approve it for merging. |
@invidian I think you said Looks OK and submitted it via request changes. So had to request again 😄 |
9fa1767
to
0948731
Compare
0948731
to
90f8b68
Compare
@knrt10 can you rebase on the current master the ARM issue will be solved. |
Change configuration for alertmanager and operator. Now user can configure it as a block. Signed-off-by: knrt10 <kautilya@kinvolk.io>
Add user-configurable tolerations for prometheus-operator and it's components. Co-authored-by: Mateusz Gozdek <mateusz@kinvolk.io> Signed-off-by: knrt10 <kautilya@kinvolk.io>
This add test for new toleration configuration introduced as block. Signed-off-by: knrt10 <kautilya@kinvolk.io>
90f8b68
to
4307660
Compare
AWS is still failing on certificate rotation part. |
finally, yes |
Add user-configurable tolerations for prometheus operator and it's
components.
Change configuration for alertmanager. Now it is configured in a
different block
closes: #1527
Signed-off-by: knrt10 kautilya@kinvolk.io
Release notes
Alert manager is not configured as a different block
Operator is also now configured as a new block