-
Notifications
You must be signed in to change notification settings - Fork 42
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
update structured logging #305
update structured logging #305
Conversation
Hi @pinikomarov. Thanks for your PR. I'm waiting for a openstack-k8s-operators member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
e59a95d
to
05d79ba
Compare
ddcf4ab
to
9878e1f
Compare
32f5f50
to
8ac6001
Compare
57e8ec5
to
dd1053d
Compare
@dprince @olliewalsh can you review please ? |
) | ||
if err != nil { | ||
return ctrl.Result{}, err | ||
} | ||
|
||
// Always patch the instance status when exiting this function so we can persist any changes. | ||
defer func() { | ||
// update the Ready condition based on the sub conditions | ||
// updatehe Ready condition based on the sub conditions |
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: s/updatehe/update the/
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.
thanks , fixed that and rebased
dd1053d
to
5876a8b
Compare
@pinikomarov this needs a rebase |
5876a8b
to
80e1b34
Compare
rebased |
Needs another rebase again :( |
80e1b34
to
b841e7e
Compare
rebased |
/ok-to-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.
/lgtm
/test keystone-operator-build-deploy-kuttl |
needs rebase |
b841e7e
to
bf4bb4b
Compare
rebased |
@pinikomarov there is one pre-commit issue which needs to be fixed to get this in |
bf4bb4b
to
bab86b1
Compare
fixed unussed ctx failing the precommit check |
oh I see now it's failing on : |
ae959a6
to
2d03fd1
Compare
2d03fd1
to
8bf05c0
Compare
@stuggi I believe fixes (and rebase) are done |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pinikomarov, stuggi 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 |
8ee0ece
into
openstack-k8s-operators:main
This automatically adds additional fields like reconcile_id etc.. from the controller context.
before :
2023-05-18T01:53:14+03:00 INFO controllers.KeystoneAPI Reconciled Service init successfully
after:
2023-10-15T02:22:33+03:00 INFO Controllers.KeystoneAPI Reconciling Service {"controller": "k eystoneapi", "controllerGroup": "keystone.openstack.org", "controllerKind": "KeystoneAPI", "KeystoneAPI": {"name":"keystone","namespace":"openstack"}, "namespace": "openstack", "name": "keystone", "reconcileID" : "30b247eb-4538-47d1-89a6-b877816a1103"}
*by using per reconcile function respective logger objects, the intention is to lay the ground for parallel reconciliation in future and avoid race conditions as ctx objects are reconcile run specific.