-
Notifications
You must be signed in to change notification settings - Fork 897
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
Upload automate models dialogs during log collection #17445
Upload automate models dialogs during log collection #17445
Conversation
cec90ae
to
3749923
Compare
This pull request is not mergeable. Please rebase and repush. |
d9a3423
to
689e1a0
Compare
Ok, this is ready for review after rebase. Note, the first commit is the 🍖 of the PR. The last 2 commits are just moving code around. |
Hold off on merging this as of yet. We are trying to be understand where the decision about "collect the automate domain/dialogs belongs"... in the depot itself (new column?), as an option of each attempt to collect logs, or a general settings key. |
f2a0acf
to
69dad37
Compare
6ce9c49
to
f693308
Compare
message.args.first[:only_current] = false | ||
expect_any_instance_of(MiqServer).to receive(:post_historical_logs).once | ||
expect_any_instance_of(MiqServer).to receive(:post_current_logs).once | ||
expect_any_instance_of(MiqServer).to receive(:post_automate_dialogs).once | ||
expect_any_instance_of(MiqServer).to receive(:post_automate_models).once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will rid this file of expect_any_instance_of
after this PR is merged. It hurts me deeply to use this thing. 😭
@ZitaNemeckova @bdunne @carbonin please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good; just a few nits.
false | ||
when nil | ||
Settings.log.collection.include_automate_models_and_dialogs | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return value unless value.nil?
Settings.log.collection.include_automate_models_and_dialogs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha, I was debating using that guard clause because it wasn't really a guard clause. I'm indifferent to which way.
flog goes from 6.5 to 5.1 so I'm good with changing it.
end | ||
msg = "#{log_type} log files from #{who_am_i} are posted" | ||
_log.info("#{log_prefix} #{msg}") | ||
task.update_status("Active", "Ok", msg) | ||
end | ||
|
||
def post_automate_models(taskid, log_depot) | ||
domain_zip = Rails.root.join("log", "domain.zip") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we put this in Rails.root.join("tmp")
or are you worried about size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was worried about the size of the file.
These tests are different in that they test the specific refactorings around task/logfile creation/update.
f693308
to
5dab507
Compare
Some comments on commits jrafanie/manageiq@6fbad9f~...5dab507 spec/models/log_file_spec.rb
|
Checked commits jrafanie/manageiq@6fbad9f~...5dab507 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 app/models/miq_server/log_management.rb
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. @bdunne anything to add?
Also, mentioned in the BZ but you can disable this feature like this: To disable this feature You can do it in the advanced settings of each server by changing the value from true to false in log/collection/include_automate_models_and_dialogs OR by script:
Confirm the changes look ok:
Then run it without the dry run option to set it:
This can be scripted for all servers by doing something like this:
|
https://bugzilla.redhat.com/show_bug.cgi?id=1535179
Depends on prior PRs:
Settings.log.collection.include_automate_models_and_dialogs
if enabled, we'll include the automate dialogs and models with log collection.If done on a Zone, the automate models and dialogs will be uploaded by one of the active appliances and will be named based on the appliance that uploaded it.