Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Delete property only if it exists #200

Merged
merged 1 commit into from
Mar 24, 2023

Conversation

ogajduse
Copy link
Member

fixes #199

In case when VM inventory != default inventory:
Broker tries to delete the inventory attribute (cached_property) even though it is not evaluated yet. That results in 'AnsibleTower' object has no attribute 'inventory'

I propose to check whether it was evaluated using __dict__ first and then safely delete it.

@ogajduse ogajduse added the bug Something isn't working label Mar 16, 2023
@ogajduse ogajduse self-assigned this Mar 16, 2023
@LadislavVasina1
Copy link

:cat:

@Griffin-Sullivan
Copy link
Collaborator

So I can test this by checking out and specifying an inventory that is not the default inventory and then try to extend it?

@ogajduse
Copy link
Member Author

So I can test this by checking out and specifying an inventory that is not the default inventory and then try to extend it?

Exactly 👍

@Griffin-Sullivan
Copy link
Collaborator

I can't seem to reproduce the error on master. Maybe you could help me edit these steps:

  1. Set default_inventory in broker_settings.yaml to rhv
  2. Checkout VM but specify osp as inventory
  3. Extend the VM

@LadislavVasina1
Copy link

@Griffin-Sullivan I have added some additional info to the issue Issue#199.
I was able to reproduce this error with these steps:

  1. Have osp set as a default in broker_settings_yaml
  2. Have any VM deployed on rhv in my inventory
  3. Try to extend (everything or just VM deployed on rhv)

Copy link
Collaborator

@Griffin-Sullivan Griffin-Sullivan left a comment

Choose a reason for hiding this comment

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

Ok I was able to reproduce the error and the PR is fixing the issue. One note about hitting this issue. If you do a checkout without a sync, you won't hit this error, but after syncing you should hit it. The reason is the tower_inventory attribute is only present in the _broker_args before the sync, but after the sync you can see tower_inventory under the host's details and the _broker_args.

@ogajduse
Copy link
Member Author

Ok I was able to reproduce the error and the PR is fixing the issue. One note about hitting this issue. If you do a checkout without a sync, you won't hit this error, but after syncing you should hit it. The reason is the tower_inventory attribute is only present in the _broker_args before the sync, but after the sync you can see tower_inventory under the host's details and the _broker_args.

@JacobCallahan That missing tower_inventory before the sync smells like a bug to me. 🤔

Copy link
Member

@JacobCallahan JacobCallahan left a comment

Choose a reason for hiding this comment

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

Nice find and solve!

@ogajduse if you want to take a shot at the specific issue @Griffin-Sullivan detailed, that would need to be something added to the initial host creation. One possible way would be to add an else to this conditional block and manually setting the tower_inventory to self.inventory (maybe with an inventory translation)
https://github.com/SatelliteQE/broker/blob/master/broker/providers/ansible_tower.py#L430

@ogajduse
Copy link
Member Author

Based on the priorities I have now, I can not dedicate more time to this. Feel free to create an issue and we can come back to it later.

@JacobCallahan JacobCallahan merged commit a8f01b9 into SatelliteQE:master Mar 24, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'AnsibleTower' object has no attribute 'inventory'
4 participants