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

feat: Bump PO to v0.66.0 #319

Merged
merged 6 commits into from
Aug 8, 2023
Merged

Conversation

marioferh
Copy link
Contributor

@marioferh marioferh commented Jul 26, 2023

Bump PO to v0.66.0

@marioferh marioferh requested a review from a team as a code owner July 26, 2023 11:07
@marioferh marioferh force-pushed the bump_PO_to_v0.66.0 branch 9 times, most recently from 15840dc to ebf97fc Compare August 1, 2023 10:21
Copy link
Contributor

@danielmellado danielmellado left a comment

Choose a reason for hiding this comment

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

Left some comments, besides that, which one are you going to move forward with? #319 or #316? There's a few nit lintings, the functions on the tests would most likely need to get moved, but looks good overall!

go.mod Outdated Show resolved Hide resolved
test/e2e/monitoring_stack_controller_test.go Outdated Show resolved Hide resolved
@marioferh marioferh force-pushed the bump_PO_to_v0.66.0 branch 3 times, most recently from 97fc1c4 to 526803f Compare August 1, 2023 17:59
@danielmellado
Copy link
Contributor

/retest checks/e2e-tests-olm

@danielmellado
Copy link
Contributor

@marioferh please rebase this PR ;)

sthaha and others added 3 commits August 8, 2023 08:49
Signed-off-by: Mario Fernandez <mariofer@redhat.com>
Signed-off-by: Mario Fernandez <mariofer@redhat.com>
@marioferh marioferh force-pushed the bump_PO_to_v0.66.0 branch 2 times, most recently from 16f768b to 7029254 Compare August 8, 2023 07:03
Signed-off-by: Mario Fernandez <mariofer@redhat.com>
Signed-off-by: Mario Fernandez <mariofer@redhat.com>
@marioferh marioferh changed the title Bump PO to v0.66.0 feat: Bump PO to v0.66.0 Aug 8, 2023
@danielmellado
Copy link
Contributor

/lgtm

Copy link
Contributor

@simonpasquier simonpasquier left a 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
Copy link
Contributor

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...

Suggested change
const CustomForeverTestTimeout = 40 * time.Second
const DefaultTestTimeout = 40 * time.Second

Copy link
Contributor Author

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,
Copy link
Contributor

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

Suggested change
For: &durationFifteenMinutes,
For: ptr.To(monv1.Duration("15m")),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix in next pr

test/e2e/monitoring_stack_controller_test.go Outdated Show resolved Hide resolved
Signed-off-by: Mario Fernandez <mariofer@redhat.com>
@simonpasquier simonpasquier enabled auto-merge (squash) August 8, 2023 11:28
@simonpasquier simonpasquier merged commit 5e42a1d into rhobs:main Aug 8, 2023
7 checks passed
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.

4 participants