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 codename in log file and stdout #17769

Merged
merged 2 commits into from
Aug 9, 2018
Merged

Conversation

mkanoor
Copy link
Contributor

@mkanoor mkanoor commented Jul 26, 2018

Our codename is not reflected in our logs or when
running it from console

In the log file it would show up as

  INFO -- : Codename:   Hammer

When running bin/rails s or bin/rails c it would show up as

  ManageIQ version: master codename: Hammer

@mkanoor
Copy link
Contributor Author

mkanoor commented Jul 26, 2018

@Fryguy @jrafanie
Please review

@@ -10,6 +10,10 @@ def self.BUILD
@EVM_BUILD ||= get_build
end

def self.CODENAME
"Hammer".freeze
Copy link
Member

Choose a reason for hiding this comment

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

why didn't you think of this for Fine? It would have been a Fine codename.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am trying to Hammer it in before its too late

Copy link
Member

Choose a reason for hiding this comment

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

@mkanoor Nailed it.

@@ -156,6 +156,7 @@ class Application < Rails::Application
config.after_initialize do
Vmdb::Initializer.init
ActiveRecord::Base.connection_pool.release_connection
puts "ManageIQ version: #{Vmdb::Appliance.VERSION} codename: #{Vmdb::Appliance.CODENAME}"
Copy link
Member

Choose a reason for hiding this comment

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

_log.info?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bdunne
When I use the log.info it dumps it into the log file but doesn't display it in stdout when you run bin/rails s

INFO -- : MIQ(Vmdb::Application.class:Application) ManageIQ version: master codename: H ammer

I was hoping to see in stdout like puma displays its version string when we run bin/rails s

  • Version 3.7.1 (ruby 2.4.4-p296), codename: Snowy Sagebrush

Should I add the log.info as well as puts for the stdout?

Copy link
Member

Choose a reason for hiding this comment

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

I have this in my database.yml...

puts "** ManageIQ #{evm_version}; Database: adapter=#{c["adapter"]}, name=#{c["database"]}, host=#{c["host"]}"

And we also have https://github.com/ManageIQ/manageiq/blob/master/config/initializers/session_store.rb#L43-L46 which gives me the following:

11:01:26 bdunne@localhost:~/projects/manageiq (master)$ rails c
** ManageIQ master; Database: adapter=postgresql, name=vmdb_development, host=
** Using session_store: ActionDispatch::Session::MemCacheStore
Loading development environment (Rails 5.0.7)
irb(main):001:0> exit

11:19:10 bdunne@localhost:~/projects/manageiq (master)$ rails s
=> Booting Puma
=> Rails 5.0.7 application starting in development on http://localhost:3000
=> Run `rails server -h` for more startup options
** ManageIQ master; Database: adapter=postgresql, name=vmdb_development, host=
** Using session_store: ActionDispatch::Session::MemCacheStore
I, [2018-07-27T11:19:28.512348 #26973]  INFO -- : Initializing websocket worker!
Puma starting in single mode...
* Version 3.7.1 (ruby 2.4.3-p205), codename: Snowy Sagebrush
* Min threads: 5, max threads: 5
* Environment: development
* Listening on tcp://localhost:3000
Use Ctrl-C to stop

Maybe we can combine all of this information somewhere?

Copy link
Member

@Fryguy Fryguy Jul 27, 2018

Choose a reason for hiding this comment

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

If you want it in Stdout, user Rails.logger instead. Rails.logger doesn't work in this context, so puts is fine.

Copy link
Member

@Fryguy Fryguy Aug 7, 2018

Choose a reason for hiding this comment

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

I thinking prefer the following text:

puts "** #{Vmdb::Application::PRODUCT_NAME} #{Vmdb::Appliance.VERSION}, codename: {Vmdb::Appliance.CODENAME}"

which would look like

** ManageIQ master, codename: Hammer

on upstream

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, we may want to move this string into a Vmdb::Appliance.BANNER method, then here just do

puts "** #{Vmdb::Appliance.BANNER}"

@Fryguy
Copy link
Member

Fryguy commented Jul 27, 2018

I generally don't like this as we have the version already dumped to the log files via the VERSION file, and I don't want to maintain a second copy of this value. When we actually cut hammer (which we haven't yet), then that will be in the VERSION file. If you're concerned about development mode, then we should probably pull from branch information.

@mkanoor
Copy link
Contributor Author

mkanoor commented Jul 31, 2018

@Fryguy The code name changes twice a year compared to the versions which happen with every zstream.
(a) This approach also helps us memorialize the codename like other products do
(b) By looking at the codename in the log file developers would know which branch to use and dont have to map version number to releases (codenames)

@Fryguy
Copy link
Member

Fryguy commented Aug 6, 2018

@mkanoor I think you misunderstand me. I don't dislike writing it to the log, I dislike hardcoding the value in a second place. We can pull this from the VERSION file, and then the VERSION file is the one source of this information.

@Fryguy
Copy link
Member

Fryguy commented Aug 6, 2018

Talked with @mkanoor and the goal is to show the codename independent of the VERSION file because of downstream and dev purposes. I have a few comments on where the stuff will live, that I'll make inline, but otherwise I'm done with this approach.

@Fryguy
Copy link
Member

Fryguy commented Aug 7, 2018

We'll also need a change to the release_branch tool of https://github.com/ManageIQ/manageiq-release

Our codename is not reflected in our logs or when
running it from console

In the log file it would show up as
  INFO -- : Codename:   Hammer
When running bin/rails s or bin/rails c it would show up as
  ManageIQ version: master codename: Hammer
@miq-bot
Copy link
Member

miq-bot commented Aug 7, 2018

Some comments on commits mkanoor/manageiq@cd911d8~...67ebcd4

config/application.rb

  • ⚠️ - 159 - Detected puts. Remove all debugging statements.

@miq-bot
Copy link
Member

miq-bot commented Aug 7, 2018

Checked commits mkanoor/manageiq@cd911d8~...67ebcd4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 3 offenses detected

config/application.rb

  • ❗ - Line 159, Col 7 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.

lib/vmdb/appliance.rb

@Fryguy Fryguy merged commit 19e608c into ManageIQ:master Aug 9, 2018
@Fryguy Fryguy added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 9, 2018
@carbonin
Copy link
Member

This breaks the region detection in the console.

ManageIQ/manageiq-appliance_console#80 is one way to fix it, but I'm wondering if there isn't some way for this output to be suppressed just for the rails runner case? Otherwise, I personally would rather remove this than add the hack to the console.

Thoughts @Fryguy @mkanoor

@tjyang
Copy link

tjyang commented Feb 8, 2019

Hi all
Any update on which approach to fix this issue ?

@carbonin
Copy link
Member

I opened an alternative solution here to just not print the codename to STDOUT in production mode. #18444

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.

8 participants