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

Remove util/vmdb-logger from appliance_console #206

Conversation

NickLaMuro
Copy link
Member

@NickLaMuro NickLaMuro commented Jun 12, 2017

This is never used by the appliance console, as it ends up just using the default ruby logger for it's default_logger:

def self.default_logger
  @default_logger ||= begin
    require 'logger'
    l = Logger.new(LOGFILE)
    l.level = Logger::INFO
    l
  end
end

default_logger is also a attr_writer on the class, so if util/vmdb-logger is wanted for the logger, a user can simply require that when loading the bin, and then assign the logger themselves onto the class variable.

QA

Confirm that the appliance console still functions just fine with this removed.

This is never used by the appliance console, as it ends up just using
the default ruby logger for it's `default_logger`:

    def self.default_logger
      @default_logger ||= begin
        require 'logger'
        l = Logger.new(LOGFILE)
        l.level = Logger::INFO
        l
      end
    end

`default_logger` is also a attr_writer on the class, so if
`util/vmdb-logger` is wanted for the logger, a user can simply require
that when loading the bin, and then assign the logger themselves onto
the class variable.
@miq-bot
Copy link
Member

miq-bot commented Jun 12, 2017

Checked commit NickLaMuro@a242ba8 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

@Fryguy
Copy link
Member

Fryguy commented Jun 13, 2017

I'm good with this, though I might argue the reverse approach that the default_logger should be changed to the vmdb-logger. @chessbyte Please review as you are extracting appliance_console, and this will collide with you.

@jrafanie
Copy link
Member

I'm ok with this. I don't think appliance_console should be required to use the vmdb_logger.

@carbonin
Copy link
Member

carbonin commented Jun 13, 2017

Just tested this out. The console still logs successfully to /var/www/miq/vmdb/log/appliance_console.log.
It very much seems like this isn't actually necessary unless there were only some options that were logging to somewhere else ...

I tested this on an appliance by configuring a database.

@NickLaMuro
Copy link
Member Author

@carbonin Thanks. Wasn't sure how to do this myself.

But yes, kinda looked like it was just there for fun, or some where there was a setup that actually assigned the ApplianceConsole::Logging.logger to Vmdb::Logger, and they didn't want to explicitly include the util/vmdb-logger in the script.

@Fryguy
Copy link
Member

Fryguy commented Jun 13, 2017

LGTM, then... letting @chessbyte coordinate the merge.

@chessbyte chessbyte merged commit b095f6f into ManageIQ:master Jun 13, 2017
@chessbyte chessbyte added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 13, 2017
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.

None yet

6 participants