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.
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 stbenjam@redhat.com > > wrote:
On Mon, Jan 16, 2017 at 3:24 AM, Ondrej Prazak oprazak@redhat.com 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.
My opinion comes as a non-core developer but more as a user. I'd prefer
not to relearn anything so any change is something I'd like to avoid.
Even when you automatically change any existing template then you still
haven't changed the knowledge inside the users head. Given this has been
the interface for quite a long time, I don't think this should be
changed lightly.
On the other hand, decoupling from the activerecord object is a good
thing. To me the proxy object sounds like an ideal middle ground because
you get the decoupling, but don't have to change the interface for the
user.
From that point of view I'd suggest to revert change and decide on the
proxy object. I also undestand that you may not have time to implement
the proxy right now, but going from an iterative perspective it is an
option to do that when a macro is added or changed. Then the interface
to the user can be formalised.
Another thing that's important to note is that those affected (users)
will most likely not even see the PR. That's a problem I don't know how
to solve but something we should all be aware of.
···
On Tue, Jan 17, 2017 at 08:52:20AM +0100, Marek Hulán wrote:
> 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 > > > > wrote:
> > > On Mon, Jan 16, 2017 at 3:24 AM, Ondrej Prazak 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 foreman-dev+unsubscribe@googlegroups.com.
> > > 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 foreman-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
