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

Add event stats output to the operator logs for Ansible based-operators. #2580

Merged
merged 1 commit into from
Feb 21, 2020
Merged

Add event stats output to the operator logs for Ansible based-operators. #2580

merged 1 commit into from
Feb 21, 2020

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Feb 19, 2020

Description
Add event stats output to the operator logs for Ansible based-operators.

Motivation
Allow we remove the ansible container ( Related to #2007 )

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 19, 2020
@camilamacedo86
Copy link
Contributor Author

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 20, 2020
@camilamacedo86 camilamacedo86 changed the title Replace the inotifywait by shell script implementation WIP - Replace the inotifywait by shell script implementation Feb 20, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 20, 2020
@camilamacedo86 camilamacedo86 changed the title WIP - Replace the inotifywait by shell script implementation Add event stats output to the operator logs for Ansible based-operators. Feb 21, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 21, 2020
@camilamacedo86 camilamacedo86 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 21, 2020
Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

I have a question.

Comment on lines +258 to +263
func printEventStats(statusEvent eventapi.StatusJobEvent) {
fmt.Printf("\n--------------------------- Ansible Task Status Event StdOut -----------------\n")
fmt.Println(statusEvent.StdOut)
fmt.Printf("\n-------------------------------------------------------------------------------\n")
}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm missing something but why do we use fmt.Printf here but logger. in other places.

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Feb 21, 2020

Choose a reason for hiding this comment

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

To print the data formatted such as we do here: #2321. It has an image which shows the output as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, if you put the ansible stdout through the logger it gets mangled completely since it is JSON serialized

Copy link
Member

@fabianvf fabianvf left a comment

Choose a reason for hiding this comment

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

/lgtm

In the long run we probably want to figure out how to better integrate the Ansible logs into the core logging facilities but I think this is an acceptable stopgap

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2020
@camilamacedo86 camilamacedo86 merged commit 03b1213 into operator-framework:master Feb 21, 2020
@camilamacedo86 camilamacedo86 deleted the solve-inotify branch February 21, 2020 18:43
camilamacedo86 added a commit that referenced this pull request Feb 25, 2020
…le logs on it (#2589)

**Description of the change:**

- Add full Ansible result output to the operator logs for Ansible based-operators configurable by EnvVar.

**Motivation for the change:**

Allow users to have the same full information that can be obtained until the version 0.15.x with the Ansible sidecar container in the operator logs. 

Note that we deprecated the inotify-tools and we will no longer scaffold the sidecar container. See #2586. Also, we have been improving the operator logs in order to attend all needs. See: #2580 and #2321.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants