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

Fix ae engine tags #555

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GilbertCherrie
Copy link
Member

@GilbertCherrie GilbertCherrie commented Aug 15, 2024

Depends on: ManageIQ/manageiq-ui-classic#9250 and ManageIQ/manageiq#23155

Fixed ae engine tag field values after this pr changed how they work: #545.

Also, updated and added new cases to ensure this tag functionality is not accidentally broken in the future.

Screenshot 2024-08-15 at 3 27 17 PM

@GilbertCherrie
Copy link
Member Author

@miq-bot assign @jrafanie
@miq-bot add-label bug

@GilbertCherrie
Copy link
Member Author

@miq-bot add-reviewer @jrafanie

@miq-bot miq-bot requested a review from jrafanie August 15, 2024 19:49
@@ -320,7 +327,13 @@ def self.create_ae_attrs(attrs, name, vmdb_object, objects = [MiqServer.my_serve
array_objects.each do |array_object|
# Each array attribute is tagged with Array:: before the attribute key unless it already starts with Array::
array_attr_key = array_object
if !array_object.starts_with?("Array::")
if !ae_attrs[array_object].empty? && ae_attrs[array_object].first.split("::").first == "Classification"
Copy link
Member

@jrafanie jrafanie Aug 15, 2024

Choose a reason for hiding this comment

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

New line 326 hurts my brain but let's not change that right now...

326 guarantees this first conditional is an array, so the empty check is fine, but, the next one is a bit concerning.

Maybe something like this:

if !ae_attrs[array_object].empty? && ae_attrs[array_object].first.to_s.split("::").first == "Classification"

if ae_attrs[array_object].first is somehow not a string, such as nil, we'll still be able to split and call first and this conditional will be false as desired.

expect(MiqAeEngine.create_automation_attributes_string(hash)).to eq(result_str)
expect(result["tags"]).to eq(result_arr)
end

Copy link
Member

Choose a reason for hiding this comment

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

@GilbertCherrie Did we completely revert the test changes so it's testing as it was before? This test looks new but others are changes. Basically, it's hard to determine the changes we're doing on code pre #545. I can't tell if you did a full revert of the prior changes in the repos and then made changes on top or we're keeping some of the changes from #545 and just fixing it to work with the original tests.

@@ -329,6 +337,9 @@ def self.create_ae_attrs(attrs, name, vmdb_object, objects = [MiqServer.my_serve
if !array_object.starts_with?("Array::")
ae_attrs.delete(array_object)
end
if !array_object.starts_with?("TagArray::") && array_attr_key.starts_with?("TagArray::")
ae_attrs.delete(array_object)
end
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what's being deleted here. It looks like it's removing non-array objects. If we need to do all of those conditions, you can combine them:

if !array_object.starts_with?("Array::") || (!array_object.starts_with?("TagArray::") && array_attr_key.starts_with?("TagArray::"))
  ae_attrs.delete(array_object)
end

@GilbertCherrie GilbertCherrie force-pushed the fix_ae_engine_tags branch 3 times, most recently from 1c8bcb8 to debff0e Compare August 21, 2024 17:59
@miq-bot
Copy link
Member

miq-bot commented Aug 21, 2024

Checked commit GilbertCherrie@0b8f603 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
3 files checked, 0 offenses detected
Everything looks fine. 🍪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants