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

Fix logger issues #955

Merged
merged 3 commits into from
Feb 28, 2017
Merged

Fix logger issues #955

merged 3 commits into from
Feb 28, 2017

Conversation

lamont-granquist
Copy link
Contributor

@lamont-granquist lamont-granquist commented Feb 28, 2017

fix ohai logger problems.

i believe this gets the logic correct in that we want to configure the
logger when we're coming from the command line but want to skip it when
we're coming from chef or from other APIs that directly construct an
Ohai::System object.

i suspect this is what thom was trying to do by moving the
Ohai::Log.init call into the Ohai::Application class (which avoids it
being called entirely when Chef creates its Ohai::System object) here:

https://github.com/chef/ohai/pull/942/files

but that change broke the behavior where we were also supposed to skip
the rest of the configure_logging method in Ohai::System when run under
chef client.

I tried going down the route of having the Ohai::Application class
construct the Ohai::System object and then having it be the
responsibility of that caller to do configure_logging work.  However,
I suspect that the initializer in Ohai::System does way too much and
that the purpose of configuring the logger where it is, is that it
must be initialized in the middle of object creation before it goes on
and creates the Loader, the Runner, creates Hints and removes constants.

So, I went the route of threading a flag through the initializer so that
Ohai::System can know if its coming from the cli or not and behave
appropriately.

There's also quite a mess with the Ohai::Log class being passed around
to the workstation loader, and it initializes itself at class loading
time and Chef::Application will inject state into Ohai::Log.  I think
the eager initialization at class loading time is to not lose early
messages when run from the cli, but it means the Ohai::Log object is always
initialized, so that isn't useful for determining if we have come from the
cli or not.

i believe this gets the logic correct in that we want to configure the
logger when we're coming from the command line but want to skip it when
we're coming from chef or from other APIs that directly construct an
Ohai::System object.

i suspect this is what thom was trying to do by moving the
Ohai::Log.init call into the Ohai::Application class (which avoids it
being called entirely when Chef creates its Ohai::System object) here:

https://github.com/chef/ohai/pull/942/files

but that change broke the behavior where we were also supposed to skip
the rest of the configure_logging method in Ohai::System when run under
chef client.

I tried going down the route of having the Ohai::Application class
construct the Ohai::System object and then having it be the
responsibility of that caller to do configure_logging work.  However,
I suspect that the initializer in Ohai::System does way too much and
that the purpose of configuring the logger where it is, is that it
must be initialized in the middle of object creation before it goes on
and creates the Loader, the Runner, creates Hints and removes constants.

So, I went the route of threading a flag through the initializer so that
Ohai::System can know if its coming from the cli or not and behave
appropriately.

There's also quite a mess with the Ohai::Log class being passed around
to the workstation loader, and it initializes itself at class loading
time and Chef::Application will inject state into Ohai::Log.  I think
the eager initialization at class loading time is to not lose early
messages when run from the cli, but it means the Ohai::Log object is always
initialized, so that isn't useful for determining if we have come from the
cli or not.

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>
Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>
@lamont-granquist
Copy link
Contributor Author

@tas50 @cheeseplus i think this is more-or-less correct

@lamont-granquist
Copy link
Contributor Author

optional hashes and named arguments in ruby #FML

tas50
tas50 previously requested changes Feb 28, 2017
Copy link
Contributor

@tas50 tas50 left a comment

Choose a reason for hiding this comment

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

Wanna add as simple test on that flag and look at the Travis failure

ruby doesn't like optional hashes combined with optional named
parameters -- because hashes and kwargs get magically converted
and ruby gets confused.

plus fixed/added specs

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>
@lamont-granquist
Copy link
Contributor Author

@tas50 @cheeseplus once more unto the breach, dear friends, once more

@tas50 tas50 merged commit cd241b6 into master Feb 28, 2017
@tas50 tas50 deleted the lcg/fix-logger branch April 4, 2017 23:20
@tas50 tas50 changed the title fix logger issues Fix logger issues Apr 11, 2017
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Bug Does not work as expected.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants