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

Improve Logs from Updating Conditions #117

Merged
merged 2 commits into from
Jan 3, 2024

Conversation

razo7
Copy link
Member

@razo7 razo7 commented Jan 2, 2024

The UpdateConditions function has an error only when an unknown agent has been found. Thus, there is no need to return and check it from reconcile, since it is not an error from trying to update it.
FAR can simply log an informative error when it was called when unknown ConditionsChangeReason.
Moreover, the lastTrinsitionTime print is more human friendly

Copy link
Contributor

openshift-ci bot commented Jan 2, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the approved label Jan 2, 2024
@razo7
Copy link
Member Author

razo7 commented Jan 2, 2024

/test 4.14-openshift-e2e
/test 4.15-openshift-e2e

1 similar comment
@razo7
Copy link
Member Author

razo7 commented Jan 2, 2024

/test 4.14-openshift-e2e
/test 4.15-openshift-e2e

@mshitrit
Copy link
Member

mshitrit commented Jan 3, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 3, 2024
@razo7 razo7 marked this pull request as ready for review January 3, 2024 06:59
@razo7
Copy link
Member Author

razo7 commented Jan 3, 2024

/retest

1 similar comment
@razo7
Copy link
Member Author

razo7 commented Jan 3, 2024

/retest

Copy link
Contributor

@clobrano clobrano left a comment

Choose a reason for hiding this comment

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

I wonder if all the error messages after UpdateStatus failures could be put inside UpdateStatus function itself. I also added some other comments in the code.

controllers/fenceagentsremediation_controller.go Outdated Show resolved Hide resolved
controllers/fenceagentsremediation_controller.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm label Jan 3, 2024
@razo7
Copy link
Member Author

razo7 commented Jan 3, 2024

I wonder if all the error messages after UpdateStatus failures could be put inside UpdateStatus function itself.

They can but this way we log an error in the function and Reconcile

@clobrano
Copy link
Contributor

clobrano commented Jan 3, 2024

I wonder if all the error messages after UpdateStatus failures could be put inside UpdateStatus function itself.

They can but this way we log an error in the function and Reconcile

Sorry, I meant inside utils.UpdateConditions.

IIUC the additional information this new logs provide is the CR's name, which is already accessible inside utils.UpdateConditions, so we could simply update the error logs that are inside utils.UpdateConditions

@razo7
Copy link
Member Author

razo7 commented Jan 3, 2024

SGTM

The function has an error when unknown agent has been found. Thus, there is no need to return and check it from reconcile, since it is not an error from trying to update it. FAR can simply log informative error when it was called when unknown ConditionsChangeReason
The time returned from LastUpdateTime was similar to '2024-01-02 15:15:55.737056215 +0000 UTC m=+19.844109800' which is less readable (and more accurate) than '2024-01-02T15:15:55.737056215Z'. The latter is good enough and better human readable IMO
@razo7 razo7 changed the title Log Error Prior to Return Error Improve Logs from Updating Conditions Jan 3, 2024
@openshift-ci openshift-ci bot added the lgtm label Jan 3, 2024
Copy link
Contributor

openshift-ci bot commented Jan 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clobrano, razo7

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@razo7
Copy link
Member Author

razo7 commented Jan 3, 2024

/retest

@openshift-merge-bot openshift-merge-bot bot merged commit 0f42131 into medik8s:main Jan 3, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants