fog / fog-vsphere

Fog for vSphere
MIT License
36 stars 63 forks source link

Remove autoload #267

Closed ezr-ondrej closed 3 years ago

ezr-ondrej commented 3 years ago

autoload is wrongly namespaced for all the models, I think it confuses Rails and causes weird issues. Issue is described here: https://community.theforeman.org/t/foreman-1-20-2-vmware-sporadic-http-500-when-querying-api-compute-resources-api/13907

@timogoebel don't you know why those autoloads are really there? All models are force required in the service class, so wrongly namespaced autoload is probably not what we need, right?

timogoebel commented 3 years ago

@timogoebel don't you know why those autoloads are really there? All models are force required in the service class, so wrongly namespaced autoload is probably not what we need, right?

Yep, I believe there should be a commit somewhere in the git history that changes this for all files. Could it be, that these are leftovers?

ezr-ondrej commented 3 years ago

Yep, I believe there should be a commit somewhere in the git history that changes this for all files. Could it be, that these are leftovers?

It was introduced in 42ae84a, but it was most likely misunderstanding of the original issue, only the first file make sense, the others at least now are definitelly wrong.

chris1984 commented 3 years ago

@ezr-ondrej does this need regression testing, or should it not break anything for downstream 6.6+

ezr-ondrej commented 3 years ago

@ezr-ondrej does this need regression testing, or should it not break anything for downstream 6.6+

I guess we should regress test this together with #264. Or just do it after merge and release?

chris1984 commented 3 years ago

I can get both of them tested together 100% by tomorrow, if that works for you?

ezr-ondrej commented 3 years ago

I can get both of them tested together 100% by tomorrow, if that works for you?

Yup :+1: that would be awesome! :+1:

ezr-ondrej commented 3 years ago

I hope we can get rid of the long lasting issue with the cache :)

plribeiro3000 commented 3 years ago

autoload has been introduced in all fog providers as a solution to an issue regarding fog loading after we started pushing providers out of the main repository.

Dunno about the issue you guys are having with autoload here but one effort we still have to complete in fog land is to change the way fog requires its dependency alltogether and leave it to only load files when needed as opposed to what it does today (load all files).

Before fog/fog#3430 it used to load all files from all providers on memory. Using autoload correctly it will load only the main file of each gem initially and the rest of the files only when needed.

The code you removed here are really not needed as fog loads them all, but have in mind that this autoload should be kept.

ezr-ondrej commented 3 years ago

The problem here is some random unitialized constants, most likely caused by the autoload being scoped under collection. So we are having autoload Fog::Vsphere::Compute::Clusters::Cluster but the real constant is Fog::Vsphere::Compute::Cluster

This is not an issue with the global Compute constant you're referencing, so we are keeping it :+1:. Although I don't think it is working as you think now. Because of this code I believe all services (in new namespaces) get loaded right after require fog-vsphere (or respective provider). @plribeiro3000 want me to take a look into that?

plribeiro3000 commented 3 years ago

@ezr-ondrej It does not work exactly as it should because we have a few providers left to drop from the main fog gem before we can actually drop that code.

So far all i'm looking in the repos is if they are ready for the next move once it happens so no need to look into it (yet).

Thanks @ezr-ondrej !

cmeissner commented 3 years ago

Is here any schedule for merging and releasing the change? Currently we have a issue at our site which is waiting for that change.