Bug #23859
Ternary operation with vm_exists? in orchestration compute
Description
In app/models/concerns/orchestration/compute.rb file there is this method:
def queue_compute
return unless compute? && errors.empty?
# Create a new VM if it doesn't already exist or update an existing vm
vm_exists? ? queue_compute_create : queue_compute_update
end
When I read the ternary operator it seems to do the exact contrary of what it is explained in the comment.
And when I test it, indeed it does not create the VM in compute provider.
I think it should be:
def queue_compute
return unless compute? && errors.empty?
# Create a new VM if it doesn't already exist or update an existing vm
!vm_exists? ? queue_compute_create : queue_compute_update
end
Then it works and it finally creates the new VM in compute provider.
I am surprised that nobody else found this before.
Maybe I misunderstood something?
Related issues
Associated revisions
Fixes #23859 - Fix vm_exists? method
Fix related tests
History
#1
Updated by Michael Moll over 1 year ago
- Related to Feature #18064: Ability to import and provision existing VMs added
#2
Updated by Michael Moll over 1 year ago
Could you open a pull request whith this, please?
Personally I'd find this easier to follow the logic:
vm_exists? ? queue_compute_update : queue_compute_create
#3
Updated by Timo Goebel over 1 year ago
We already have a fix for this as part of https://github.com/theforeman/foreman/pull/5569.
#4
Updated by Michael Moll over 1 year ago
I'm unsure about the actual impact of this bug.
If it's relevant, it would good to have it as a separate commit to cherry-pick it down to the supported releases.
#5
Updated by The Foreman Bot over 1 year ago
- Status changed from New to Ready For Testing
- Pull request https://github.com/theforeman/foreman/pull/5675 added
#6
Updated by Tristan Robert over 1 year ago
The PR refactors vm_exists? but my original issue still remains. I will make another issue (feature enhancement exactly) and a new PR.
#7
Updated by Michael Moll over 1 year ago
- Legacy Backlogs Release (now unused) set to 353
#8
Updated by Tristan Robert over 1 year ago
- % Done changed from 0 to 100
- Status changed from Ready For Testing to Closed
Applied in changeset 2a6bce4de57782966e6f5aaaaf415756fe981c0f.
#9
Updated by Ewoud Kohl van Wijngaarden over 1 year ago
- Triaged set to No
- Category set to Compute resources
Fixes #23859 - Queue orchestration compute