Hello The discussion has diverged into something different. People agree we should add an extra layer but implemented through proxy objects. It's also clear no one starts actively working on this right now. At the same time I want to add new macro, let's say rpm_distro? which returns true/false based on @host.operatingsystem, how do I do it?
So far the answer was to add a new macro. Now with the proxy object, should I define it in host so one would use @host.rpm_distro?. Honestly I don't want to add more stuff into Host object, not even through concerns. So should I wait until we have a proxy layer? That means I can't add any improvements. For me the best answer is add new macro called "rpm_distro?". This will result in two approaches in future, we'll have both @host.param and rpm_distro? I think the best approach is just reverting the deprecation warning and add a value to host_param macro. It can check that user is asking for a parameter that does not exist and raise a exception of a specific class so rendering could display nicer error message such as "You tried to use parameter with name abc but it's not defined for host xyz" instead of "Undefined method upcase on nil". We could extend Host#param for this too but it's not right, this exception is specific for template rendering, other internal usage of this method might rely on fact it returns nil. My probably last comment in this thread - the PR was opened Nov 1st 2016, deprecation was suggested Nov 2nd, merged Jan 3rd 2017 [1]. I know a lot of devs were offline during December but still, please try to raise you concerns in PRs rather than revert stuff. [1] https://github.com/theforeman/foreman/pull/3983 -- Marek On pondělí 16. ledna 2017 14:59:43 CET Tomer Brisker wrote: > Hi, > I think that while there are benefits to moving away from the host object, > we have a de-facto API based on it. > A way to change without breaking user's template would be to use a "proxy" > object that maintains the same endpoints and allows adding functionality or > handling changes in the underlying objects without breaking the way users > use them. > So the templates will still have a "@host" object with the same methods as > before, except it will in fact be proxy object and not an active record. > For example, we could have a nice error message if the actual host is nil. > We could also leverage method_missing to easily handle passing anything not > defined in the api to the host (possibly only if safe mode is off), so that > any users relying on active record methods etc. won't have breakage. > > Tomer > > On Mon, Jan 16, 2017 at 2:49 PM, Stephen Benjamin <[email protected]> > > wrote: > > On Mon, Jan 16, 2017 at 3:24 AM, Ondrej Prazak <[email protected]> wrote: > > > +1 for keeping the macros. IMHO, just because we did something a certain > > > > way for a long time should not prevent us from changing it if there are > > reasons for a change. This is also not the first change in core that > > affected plugin(s) in a negative way and I doubt it will be the last. > > Breaking plugin tests is unpleasant, but it is still a second best > > scenario. > > > > > > The plugin test failure is relatively minor, the main objection here > > is the number of users who rely on this interface now need to change. > > After raising this issue we got some PR's that help the migrations > > which is nice, but there's > > organizations who have less sophisticated technical users who learned > > basic ERB who have to re-learn this, not to mention that git repos of > > users using foreman_templates now have to update, and we now have lots > > of incorrect info out there on the internet and internal intranets > > that need to be updated. We're making our users do a lot of work for > > not a lot of good. > > > > The change was not needed, and it was merged while a significant > > number of developers were away for the holidays. I think it should be > > changed to restore the @host interface, or reverted. I know there are > > some benefits to stop using the Host::Managed object, but we could've > > implemented @host as an instance of some proxy class without breakage > > for users. > > > > FWIW, this change, if we actually followed semver, should require a > > bump to 2.0 once the deprecated methods are removed. > > > > - Stephen > > > > -- > > You received this message because you are subscribed to the Google Groups > > "foreman-dev" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to [email protected]. > > For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups "foreman-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
