-
Notifications
You must be signed in to change notification settings - Fork 58
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
feat: Bump PO to v0.66.0 #319
Conversation
15840dc
to
ebf97fc
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.
97fc1c4
to
526803f
Compare
/retest checks/e2e-tests-olm |
@marioferh please rebase this PR ;) |
Signed-off-by: Mario Fernandez <mariofer@redhat.com>
Signed-off-by: Mario Fernandez <mariofer@redhat.com>
16f768b
to
7029254
Compare
3296300
to
427c5d9
Compare
Signed-off-by: Mario Fernandez <mariofer@redhat.com>
427c5d9
to
9890ab5
Compare
Signed-off-by: Mario Fernandez <mariofer@redhat.com>
9890ab5
to
e445132
Compare
/lgtm |
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 a few nits 🎉
@@ -23,6 +23,8 @@ import ( | |||
"k8s.io/apimachinery/pkg/util/wait" | |||
) | |||
|
|||
const CustomForeverTestTimeout = 40 * time.Second |
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.
(nit) feel free to ignore but I'd name the constant in an explicit manner for clarity...
const CustomForeverTestTimeout = 40 * time.Second | |
const DefaultTestTimeout = 40 * time.Second |
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.
fix in next pr
@@ -53,7 +54,7 @@ func newSinglePrometheusRule(t *testing.T, name, expr string) *monv1.PrometheusR | |||
Rules: []monv1.Rule{{ | |||
Alert: "alert name", | |||
Expr: intstr.FromString(expr), | |||
For: "15m", | |||
For: &durationFifteenMinutes, |
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.
suggestion for a follow-up PR
For: &durationFifteenMinutes, | |
For: ptr.To(monv1.Duration("15m")), |
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.
fix in next pr
Signed-off-by: Mario Fernandez <mariofer@redhat.com>
Bump PO to v0.66.0