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

remove writing out lastCheckedAt status #300

Merged
merged 1 commit into from
Nov 8, 2024
Merged

Conversation

maleck13
Copy link
Collaborator

@maleck13 maleck13 commented Nov 7, 2024

Signed-off-by: craig [email protected]

rh-pre-commit.version: 2.2.0
rh-pre-commit.check-secrets: ENABLED

@maleck13 maleck13 force-pushed the remove-last-checked branch 2 times, most recently from 682db9c to 8ea0c47 Compare November 7, 2024 16:10
Signed-off-by: craig <[email protected]>

rh-pre-commit.version: 2.2.0
rh-pre-commit.check-secrets: ENABLED
@maleck13 maleck13 force-pushed the remove-last-checked branch from 8ea0c47 to db6233f Compare November 8, 2024 09:18
@@ -246,11 +252,11 @@ func (w *Probe) Start(clientctx context.Context, k8sClient client.Client, probe
} else {
freshProbe.Status.ConsecutiveFailures = 0
}
logger.V(1).Info("health: execution complete ", "result", probeResult, "checked at", probeResult.CheckedAt.String(), "previoud check at ", probeResult.PreviousCheck)
freshProbe.Status.Healthy = &probeResult.Healthy
freshProbe.Status.LastCheckedAt = probeResult.CheckedAt
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should not write LastCheckedAt to the probe CR anymore

@@ -26,10 +26,13 @@ const (
)

type ProbeResult struct {
// CheckedAt the current helath check time
CheckedAt metav1.Time
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this value any more? I thought that the only that matters is LastCheckedAt in the localProbe 🤔

Copy link
Contributor

@maksymvavilov maksymvavilov left a comment

Choose a reason for hiding this comment

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

Seems like it is working
image
The CR has no LAstCheckedAt field even when attempting to write it. No errors as well.
It might be easier to remove LastCheckedAt completely from the probe CRD and have it as a local var. Or it will make things even more complicated 🤔
In any case - it is working

@maleck13
Copy link
Collaborator Author

maleck13 commented Nov 8, 2024

I will merge as is and then we can enhance it more afterwards as this will stop a lot of events happening anyway

@maleck13 maleck13 added this pull request to the merge queue Nov 8, 2024
Merged via the queue into main with commit a0cca97 Nov 8, 2024
13 of 15 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.

2 participants