Fixes #38272 - Use .presence for ip6 address #10470
Conversation
| @@ -99,6 +99,6 @@ def validate_ssh_provisioning | |||
|
|
|||
| def provision_host | |||
| # usually cloud compute resources provide IPs but virtualization do not | |||
| provision_interface.ip6 || provision_interface.ip || provision_interface.fqdn | |||
There was a problem hiding this comment.
You can use .ip6.presence to force an empty string to nil and keep ||
ShimShtein
force-pushed
the
fix_empty_string_ip6
branch
from
cc0ef10 to
9c13f1a
Compare
ShimShtein
changed the title
|
@ShimShtein |
There was a problem hiding this comment.
Something I didn't check before, but this was introduced in 820308f and https://projects.theforeman.org/issues/38057 is fixed in 3.14. That means you can use Refs #38057 here if we cherry pick it to 3.14 before GA.
I see you also used provision_interface.ip.presence which makes me think it never could reach provision_interface.fqdn to begin with. That part was introduced in 8397b3c but looks like that was flawed to begin with. That could be an argument for its own issue.
What do you think?
|
Well, I think it's a bit semantic, and in this case I would leave it as a separate issue. It does make me wonder though, why should we use an empty string for IP values anyway? If the value is missing, it should be nil. If I had some spare time on my hands, I would look into making sure it's either IP or nil. |
There was a problem hiding this comment.
It does make me wonder though, why should we use an empty string for IP values anyway? If the value is missing, it should be nil. If I had some spare time on my hands, I would look into making sure it's either IP or nil.
I know Django generally has the rule that you shouldn't use nullable strings because now there are 2 representations for an empty value. They generally recommend empty strings for empty. Perhaps it helps a bit that in Python an empty string is false where in Ruby it's true.
Do you know what sort of guidelines Rails provides here?
I'm ok merging this because it fixes a regression in 3.14 (so we need to merge it this week) and since it does make the change to .ip as well I think a separate issue makes sense.
Could you please make sure the commit message and PR title accurately describe the chosen solution? After that ACK.
We cannot use regular `||` method on `ip6` and `ip` fields, since the value can be an empty string.
ShimShtein
force-pushed
the
fix_empty_string_ip6
branch
from
9c13f1a to
79a5deb
Compare
|
Synced commit and PR |
|
Cherry-pick: #10475 |
Stems from the fact that ip6 is defined as an empty string here: https://github.com/theforeman/foreman/blob/bc335dcd2709ee85b20c0af079f06768527c53d2/app/models/concerns/orchestration/ssh_provision.rb#L102C28-L102C36