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

Created log_object method in System/CommonMethods/Utils class. #362

Merged
merged 1 commit into from
Jul 31, 2018

Conversation

billfitzgerald0120
Copy link
Contributor

@billfitzgerald0120 billfitzgerald0120 commented Jul 16, 2018

This is a new method which can then be included into the other methods using the Embedded method feature. Originally this was dump_root but the name was changed to log_object.
This method will allow logging of root, current and any object passed to it.

#350

@miq-bot add_label enhancement
@miq-bot assign @gmcculloug

$evm.log("info", "")
end

dump_root
Copy link
Member

Choose a reason for hiding this comment

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

Since the goal is to use this as an embedded method we do not want the method to be called from here. The automate method that embeds it would inject the call where needed.

Also need to consider moving it to another namespace in Automate where other common embedded methods would live.

cc @tinaafitz @mkanoor

Copy link
Member

Choose a reason for hiding this comment

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

@billfitzgerald0120 Can we add a new class for these types of utilities in /System/CommonMethods/Utils?

Copy link
Member

@tinaafitz tinaafitz Jul 16, 2018

Choose a reason for hiding this comment

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

@billfitzgerald0120 The method should have the module information.

  module Automate
    module System
      module CommonMethods
        module Utils

Copy link
Member

@tinaafitz tinaafitz left a comment

Choose a reason for hiding this comment

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

@billfitzgerald0120 Can we add the module information and move this method to the new class /System/CommonMethods/Utils?

# Description: Log all attributes stored in the $evm.root hash.
#

def dump_root
Copy link
Contributor

Choose a reason for hiding this comment

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

@billfitzgerald0120 to aid in testing we need to be able to inject dependencies. we won't have a $evm when we test
so this might be a better way of accomplishing the same thing

def dump_root(handle = $evm)
    handle.log("info", "Root:<handle.root> Attributes - Begin")
    handle.root.attributes.sort.each { |k, v| handle.log("info", "  Attribute - #{k}: #{v}") }
    handle.log("info", "Root:<handle.root> Attributes - End")
    handle.log("info", "")
 end

@billfitzgerald0120 billfitzgerald0120 changed the title Created DumpRoot instance and method in System/Request class. Created dump_root method in System/CommonMethods/Utils class. Jul 17, 2018
@mkanoor
Copy link
Contributor

mkanoor commented Jul 17, 2018

@billfitzgerald0120 can you add a spec for this.
You can look at this https://gist.github.com/jeffwarnica/bfc885ab7bd7197d626a55fa81913cc5 for some ideas on how to implement it

# Description: Log all attributes stored in the $evm.root hash.
#

def dump_root(handle = $evm)
Copy link
Contributor

Choose a reason for hiding this comment

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

@billfitzgerald0120 this needs the modules and also the name of a class preferably Utils.
And the dump_root should be a class method

def self.dump_root(handle = $evm)
....
end

@billfitzgerald0120 billfitzgerald0120 changed the title Created dump_root method in System/CommonMethods/Utils class. Created log_object method in System/CommonMethods/Utils class. Jul 27, 2018
@gmcculloug
Copy link
Member

@billfitzgerald0120 The branch this is built on is dump_root and the PR title use to reference that, but now this is all logging methods. I would have expected this to be a new PR to add more common functionality, not remove the dump_root logic. I'm confused. 😕

@billfitzgerald0120
Copy link
Contributor Author

@gmcculloug @mkanoor @tinaafitz Madhu and I changed dump_root to log_object yesterday. I will change this to WIP. We need to discuss with Tina and Madhu.

@billfitzgerald0120 billfitzgerald0120 changed the title Created log_object method in System/CommonMethods/Utils class. [WIP] Created log_object method in System/CommonMethods/Utils class. Jul 28, 2018
@miq-bot miq-bot added the wip label Jul 28, 2018
@gmcculloug
Copy link
Member

I totally misread the code change, I see now what you are doing. Sorry about that. 😊

@billfitzgerald0120 billfitzgerald0120 changed the title [WIP] Created log_object method in System/CommonMethods/Utils class. Created log_object method in System/CommonMethods/Utils class. Jul 30, 2018
@billfitzgerald0120
Copy link
Contributor Author

@tinaafitz Please Review

@miq-bot miq-bot removed the wip label Jul 30, 2018
Copy link
Member

@tinaafitz tinaafitz left a comment

Choose a reason for hiding this comment

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

@billfitzgerald0120 Looks good.
@mkanoor Please review.

This is a new method which can then be included into the other methods using the Embedded method feature.
Created System/CommonMethods/Utils class.

ManageIQ#350

Changed name to LogObject and many other changes .....
Added documentation in the method
Fixed extra line
Fixed rubocop errors in spec
@miq-bot
Copy link
Member

miq-bot commented Jul 31, 2018

Checked commit billfitzgerald0120@10f0ca6 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@mkanoor mkanoor merged commit 5dd26d1 into ManageIQ:master Jul 31, 2018
@mkanoor mkanoor added this to the Sprint 92 Ending Aug 13, 2018 milestone Jul 31, 2018
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.

5 participants