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 test for legacy MiqReport #18699
base: master
Are you sure you want to change the base?
Remove test for legacy MiqReport #18699
Conversation
Checked commit romanblanco@1435fe3 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 spec/models/miq_report/import_export_spec.rb
|
Do we just not support legacy reports anymore via upgrade? Does it make sense to just have a migration to ensure this? (I'm not even sure how old this is...if it's older than, C-release I think (whatever the version you are forced to upgrade to), then this is probably ok. cc @gtanzillo ? |
This commit 7c78517f3e2 is from 2013 via @lfu I do remember running into this scenario years back, but can't place why. |
Waiting on response from @gtanzillo |
To be honest, I'm not familiar with the issue that the "legacy report" check was meant to address. I do recall that in the distant past we had an issue with ActiveRecord objects that were serialized in report results and needed some special code to deal with then when the internal AR object format was changed. But this is in the report definition. |
The only concern I have is when we export a bunch of miq_reports in a single yaml file. I thought this was needed to import it back again. That is the only use case I know of. I don't know how to export or import them, I just know we had this functionality within the past few years. If that still works then I'm 👍 |
@@ -27,7 +27,6 @@ def resolve_view_path(file_name, file_name_no_suffix = nil) | |||
def import_from_hash(report, options = nil) | |||
raise _("No Report to Import") if report.nil? | |||
|
|||
report = report["MiqReport"] if report.keys.first == "MiqReport" |
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.
this is here to support importing a yaml file with multiple reports in them. those seem to have the extra MiqReport
nodes.
as long as we no longer support exporting all reports into a single yaml file, then this should be fine. I don't know the import/export parts well enough to verify my statement.
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation. |
Removed special handling of legacy
MiqReport
while importing