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

Consolidate logfile naming and destination filename #17438

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented May 17, 2018

https://bugzilla.redhat.com/show_bug.cgi?id=1535179

Depends on prior PRs:

LogFile#name was generating a name that was similar but yet different
than the destination_file_name. This name is not displayed or used in
the UI, so we can push the logic of building the destination_file_name
into the name of the LogFile, which is exactly the place we need it to
differentiate current/historical/automate models/automate dialogs.

@@ -180,10 +180,17 @@ def destination_directory
File.join("#{resource.zone.name}_#{resource.zone.id}", "#{resource.name}_#{resource.id}")
end

def self.logfile_name(resource, category = "Current", date_string = nil)
region = MiqRegion.my_region.try(:region) || "unknown"
[category, "region", region, resource.zone.name, resource.zone.id, resource.name, resource.id, date_string].compact.join(" ")
Copy link
Member

Choose a reason for hiding this comment

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

Why the .compact? Could anything be nil?

Copy link
Member Author

@jrafanie jrafanie May 21, 2018

Choose a reason for hiding this comment

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

logfile_name is being used for non-log related materials such as automate dialogs/domains. date_string will be nil for them, but also in the unlikely event we couldn't find timestamps in the evm.log or compressed version of the evm.log.

end

def name
super || self.class.logfile_name(resource)
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to use the underscorized version?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really. I was trying to minimize change of the LogFile#name and it made sense to use underscores for a filename but didn't matter for LogFile#name.

https://bugzilla.redhat.com/show_bug.cgi?id=1535179

Builds on top of the refactoring of current/historical log collection
found in:
ManageIQ#17437

LogFile#name was generating a name that was similar but yet different
than the destination_file_name.  This name is not displayed or used in
the UI, so we can push the logic of building the destination_file_name
into the name of the LogFile, which is exactly the place we need it to
differentiate current/historical/automate models/automate dialogs.
@jrafanie jrafanie force-pushed the consolidate_logfile_naming_and_destination_filename branch from 5133cb0 to 53f2b84 Compare May 21, 2018 16:04
@jrafanie jrafanie changed the title [WIP] Consolidate logfile naming and destination filename Consolidate logfile naming and destination filename May 21, 2018
@jrafanie jrafanie removed the wip label May 21, 2018
@miq-bot
Copy link
Member

miq-bot commented May 21, 2018

Checked commits jrafanie/manageiq@3625847~...53f2b84 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 4 offenses detected

spec/models/file_depot_ftp_spec.rb

spec/models/miq_server/log_management_spec.rb

@jrafanie
Copy link
Member Author

This is now ready to go after rebasing. WIP removed. Note, the COPs will be addressed in #17445

@bdunne bdunne merged commit 1e73ab0 into ManageIQ:master May 21, 2018
@bdunne bdunne added this to the Sprint 86 Ending May 21, 2018 milestone May 21, 2018
@bdunne bdunne self-assigned this May 21, 2018
@jrafanie jrafanie deleted the consolidate_logfile_naming_and_destination_filename branch May 21, 2018 17:26
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.

3 participants