-
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
Remove new fields from export if empty #17080
Conversation
To stay backward compatible with old importers, if the 2 new attributes options embedded_methods are empty, then we should not export it. This wold help if we try to export the Automate model from newer versions, which dont have * Embedded Methods or * Embedded Ansible methods, which use options into an older system.
@tinaafitz @gmcculloug @lfu |
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.
@mkanoor Looks good. This changes makes a lot of sense.
spec/models/miq_ae_method_spec.rb
Outdated
expect(result['name']).to eql('foo_method') | ||
expect(result['location']).to eql('inline') | ||
expect(result['options']).to be_nil | ||
expect(result['embedded_methods']).to be_nil |
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.
We should check that the result
hash does not have to key instead of the accessor method returning nil
.
Checked commits mkanoor/manageiq@4ee8834~...a4156e5 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0 |
@mkanoor Please add BZ link. |
To stay backward compatible with old importers, if the 2 new attributes from G release
are empty, then we should not export it. This would help if we try
to export the Automate model from newer versions, which dont have
into an older system. The old importer wouldn't be aware of these new attributes.
Issue found when back porting fixes to Fine release PR # ManageIQ/manageiq-content#256