-
Notifications
You must be signed in to change notification settings - Fork 36
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 MiqTempfile initialize kwargs error #198
Conversation
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.
Should we add specs too? (not sure it's worth it)
FWIW, CI only shows support for 3.0 and 3.1, so this should be fine. Also this is only called from one place in our entire org and it's right in this gem.
Yeah I started looking at this but I don't know that I could show the failure in a spec without actually calling Tempfile.new and hitting the filesystem
Interesting maybe we could drop this and just use Tempfile normally in the caller |
Yeah, I'm not sure. I think the idea, based on the comment, was to have a single place to defined the /var/www/miq_tmp dir and be sure all callers are using it consistently, but the path is all over the code base. |
Right the path is referenced in a number of places but Openstack SSA seems to be the only place actually using it, the rest of those are around configuring a disk (appliance_console) or monitoring the disk usage. Based on the original commit message
It seems like the purpose of this class was to use this directory if it was configured, not just if it existed. Maybe somewhere along the line we pre-created the directory even if a disk wasn't mounted? Either way even back then it seems openstack ssa was the only caller of this class. |
It seems like on an appliance we will always have a /var/www/miq_tmp mountpoint https://github.com/ManageIQ/manageiq-appliance-build/blob/master/kickstarts/partials/main/disk_layout.ks.erb#L7 so the "if /var/www/miq_tmp exists" doesn't make sense? |
I think the backup is more about non-appliance development and possibly just a safety net - better to have a backup than try to use a non-existent dir. |
Merging anyway cause this fixes a bug. I am very curious how this is supposed to work on containerized though. |
Fixed: - Fix MiqTempfile initialize kwargs error (#198)
Tempfile
takes a**options
parameter which we were passing in as splat-ed positional parameters causing this error:With this change applied:
ManageIQ/manageiq-providers-openstack#890