Re: [foreman-dev] Revert removal of @host.params for host_param

[Ewoud Kohl van Wijngaarden]

Hello,

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 

Re: [foreman-dev] Revert removal of @host.params for host_param

[Marek Hulán]

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+unsubscr...@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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Re: [foreman-dev] Revert removal of @host.params for host_param

[Tomer Brisker]

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+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>



-- 
Have a nice day,
Tomer Brisker
Red Hat Engineering

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [foreman-dev] Revert removal of @host.params for host_param

[Lukas Zapletal]

I am also for keeping the change but giving more attention to the
upgrades and providing some tooling to convert existing templates.

LZ

On Wed, Jan 11, 2017 at 5:05 PM, Daniel Lobato Garcia
 wrote:
> Hi foreman devs,
>
> Just noticed today https://github.com/theforeman/foreman/pull/3983/files
> after some comments on IRC. What's the background behind this change?
>
> As far as I can see, this merely moves
>
> @host.param to host_param
> @host.param_true? to host_param_true?
> @host.param_false? to host_param_false?
> @host.info to host_enc
>
> without gaining anything from the change. This will force people to
> change their templates (including our community templates) when the
> deprecation is removed, and there's nothing to gain.
>
> Does someone know what's the rationale behind this change? As it stands
> right now, I propose reverting that commit entirely to avoid inflicting
> that pain on users - which include many devs who maintain templates.
>
> Best,
>
> --
> Daniel Lobato Garcia
>
> @dLobatog
> blog.daniellobato.me
> daniellobato.me
>
> GPG: http://keys.gnupg.net/pks/lookup?op=get=0x7A92D6DD38D6DE30
> Keybase: https://keybase.io/elobato
>
> --
> 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+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.



-- 
Later,
  Lukas @lzap Zapletal

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Re: [foreman-dev] Revert removal of @host.params for host_param

[Ondrej Prazak]

+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.

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Re: [foreman-dev] Revert removal of @host.params for host_param

[Adam Ruzicka]

I'm with Marek on this one. Anything giving us more breathing space
is a good thing in the long run, even if the immediate effect might be a
bit negative.
A migration for community-templates would be nice though.

The mentioned REX issue was just a minor inconvenience. It just stopped the
tests
for a few days.


Adam

On Fri, Jan 13, 2017 at 7:09 PM, Marek Hulan  wrote:

> I don't think it causes any problems. I don't see the reason why the whole
> commit should be reverted. If something then perhaps deprecation warning
> could be removed. I'd still prefer communuty-templates using macros instead
> of internal objects.
>
> --
> Marek
>
> Odesláno pomocí AquaMail pro Android
> http://www.aqua-mail.com
>
> Dne 13. ledna 2017 18:27:13 "Sean O'Keeffe"  napsal:
>
>> Maybe the best thing to do for now it to revert it and send a PR to the
>> RFC repo for a proper discussion?
>>
>> On Fri, Jan 13, 2017 at 2:26 PM, Daniel Lobato Garcia <
>> elobat...@gmail.com> wrote:
>>
>>> On 01/12, Marek Hulán wrote:
>>>  > >
>>> > > > I strongly disagree that this does not have big benefits. Using
>>> internal
>>> > > > Foreman objects in templates is a bad practice. It blocks us from
>>> > > > improving
>>> > > > our code. Therefore it's very important to build a DSL that users
>>> can use
>>> > > > in templates and keep that compatible. We can then later change the
>>> > > > implementation of host_param_true method without any templates
>>> changing.
>>> > > > Think
>>> > > > of this as a templating API.
>>> > > >
>>> > > > Another less significant benefit is that for plugins it's easier to
>>> > > > wrap/alias
>>> > > > the template method rather than manipulating something that's used
>>> > > > internally in @host. Still not ideal but that should be solved by
>>> > > > https://github.com/ theforeman/foreman/pull/3701
>>> > > >
>>> > > > Of course it will require users to migrate to new template helpers
>>> which
>>> > > > is
>>> > > > why we move there slowly and hopefully with proper deprecations. I
>>> was
>>> > > > hoping to do the update for community-templates since it's very
>>> easy
>>> > > > migration.
>>> > > >
>>> > > > If you think it's too complicated for users we could provide rake
>>> task or
>>> > > > migration. But please don't revert this.
>>> > >
>>> > > Yes, if you provided a migration it would be much better.  That
>>> doesn't
>>> > > really solve the problem with people using the foreman_templates
>>> plugin
>>> > > who will have those changes reverted, but I guess it's better than
>>> nothing.
>>> > >
>>> > > There's still dozens of other things allowed for @host in the Jail
>>> that
>>> > > aren't covered by these macros.   What's the plan for those?
>>> >
>>> > Whenever we have a chance, we should move from internal objects to
>>> macros. The
>>> > more macros we have the higher the chance is that we can keep templates
>>> > compatible.
>>>
>>> Sorry but I oppose this. Some internal objects are quite stable, in
>>> fact people rely upon them successfully - but we break the compatibility
>>> for apparent advantages that IMO are not worth the hassle.
>>>
>>> @host.operatingsystem, @host.architecture, etc... and @host.params, are
>>> very stable objects that can safely be called from templates and expect
>>> that will work for a long time. Not only our users do it. We also relied
>>> upon these internal objects for years.
>>>
>>> The first community templates PRs which are 4 years old used
>>> @host.params, @host.operatingsystem, etc.. Those are stable enough to
>>> not warrant writing a macro for them
>>>
>>> What we did on #16740 is basically renaming things around and deprecating
>>> params for macros that don't do anything but calling @host.params
>>> themselves.
>>>
>>> I would argue for the opposite way of thinking. When we change how
>>> these internal APIs work, we should deprecate them. If these APIs
>>> remain stable, don't change anything as it provides no value, and brings
>>> headaches and wastes time. Time wasted on writing this thread, on the
>>> people who write and review PRs in the project and associated ones
>>> (templates, the migration to new macros, fixing anything that might
>>> break,
>>> and adding more LOC which complicate our codebase).
>>>
>>> >
>>> > > I still think this provides more headaches than any benefits.
>>> >
>>> > I hope that following helps
>>> > * https://github.com/theforeman/community-templates/pull/343
>>> > * https://github.com/theforeman/foreman/pull/4187
>>> > * https://gist.github.com/ares/5435226ef0317613535101765404d3f5
>>> >
>>> > --
>>> > Marek
>>> >
>>> > >
>>> > >
>>> > > - Stephen
>>> > >
>>> > > > --
>>> > > > Marek
>>> > > >
>>> > > > --
>>> > > > 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+unsubscr...@googlegroups.com.
>>> > 

Re: Re: [foreman-dev] Revert removal of @host.params for host_param

[Marek Hulan]

I don't think it causes any problems. I don't see the reason why the whole 
commit should be reverted. If something then perhaps deprecation warning 
could be removed. I'd still prefer communuty-templates using macros instead 
of internal objects.


--
Marek

Odesláno pomocí AquaMail pro Android
http://www.aqua-mail.com


Dne 13. ledna 2017 18:27:13 "Sean O'Keeffe"  napsal:


Maybe the best thing to do for now it to revert it and send a PR to the RFC
repo for a proper discussion?

On Fri, Jan 13, 2017 at 2:26 PM, Daniel Lobato Garcia 
wrote:


On 01/12, Marek Hulán wrote:
 > >
> > > I strongly disagree that this does not have big benefits. Using
internal
> > > Foreman objects in templates is a bad practice. It blocks us from
> > > improving
> > > our code. Therefore it's very important to build a DSL that users
can use
> > > in templates and keep that compatible. We can then later change the
> > > implementation of host_param_true method without any templates
changing.
> > > Think
> > > of this as a templating API.
> > >
> > > Another less significant benefit is that for plugins it's easier to
> > > wrap/alias
> > > the template method rather than manipulating something that's used
> > > internally in @host. Still not ideal but that should be solved by
> > > https://github.com/ theforeman/foreman/pull/3701
> > >
> > > Of course it will require users to migrate to new template helpers
which
> > > is
> > > why we move there slowly and hopefully with proper deprecations. I
was
> > > hoping to do the update for community-templates since it's very easy
> > > migration.
> > >
> > > If you think it's too complicated for users we could provide rake
task or
> > > migration. But please don't revert this.
> >
> > Yes, if you provided a migration it would be much better.  That doesn't
> > really solve the problem with people using the foreman_templates plugin
> > who will have those changes reverted, but I guess it's better than
nothing.
> >
> > There's still dozens of other things allowed for @host in the Jail that
> > aren't covered by these macros.   What's the plan for those?
>
> Whenever we have a chance, we should move from internal objects to
macros. The
> more macros we have the higher the chance is that we can keep templates
> compatible.

Sorry but I oppose this. Some internal objects are quite stable, in
fact people rely upon them successfully - but we break the compatibility
for apparent advantages that IMO are not worth the hassle.

@host.operatingsystem, @host.architecture, etc... and @host.params, are
very stable objects that can safely be called from templates and expect
that will work for a long time. Not only our users do it. We also relied
upon these internal objects for years.

The first community templates PRs which are 4 years old used
@host.params, @host.operatingsystem, etc.. Those are stable enough to
not warrant writing a macro for them

What we did on #16740 is basically renaming things around and deprecating
params for macros that don't do anything but calling @host.params
themselves.

I would argue for the opposite way of thinking. When we change how
these internal APIs work, we should deprecate them. If these APIs
remain stable, don't change anything as it provides no value, and brings
headaches and wastes time. Time wasted on writing this thread, on the
people who write and review PRs in the project and associated ones
(templates, the migration to new macros, fixing anything that might break,
and adding more LOC which complicate our codebase).

>
> > I still think this provides more headaches than any benefits.
>
> I hope that following helps
> * https://github.com/theforeman/community-templates/pull/343
> * https://github.com/theforeman/foreman/pull/4187
> * https://gist.github.com/ares/5435226ef0317613535101765404d3f5
>
> --
> Marek
>
> >
> >
> > - Stephen
> >
> > > --
> > > Marek
> > >
> > > --
> > > 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+unsubscr...@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+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

--
Daniel Lobato Garcia

@dLobatog
blog.daniellobato.me
daniellobato.me

GPG: http://keys.gnupg.net/pks/lookup?op=get=0x7A92D6DD38D6DE30
Keybase: https://keybase.io/elobato

--
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.



--

Re: Re: [foreman-dev] Revert removal of @host.params for host_param

[Sean O'Keeffe]

Maybe the best thing to do for now it to revert it and send a PR to the RFC
repo for a proper discussion?

On Fri, Jan 13, 2017 at 2:26 PM, Daniel Lobato Garcia 
wrote:

> On 01/12, Marek Hulán wrote:
>  > >
> > > > I strongly disagree that this does not have big benefits. Using
> internal
> > > > Foreman objects in templates is a bad practice. It blocks us from
> > > > improving
> > > > our code. Therefore it's very important to build a DSL that users
> can use
> > > > in templates and keep that compatible. We can then later change the
> > > > implementation of host_param_true method without any templates
> changing.
> > > > Think
> > > > of this as a templating API.
> > > >
> > > > Another less significant benefit is that for plugins it's easier to
> > > > wrap/alias
> > > > the template method rather than manipulating something that's used
> > > > internally in @host. Still not ideal but that should be solved by
> > > > https://github.com/ theforeman/foreman/pull/3701
> > > >
> > > > Of course it will require users to migrate to new template helpers
> which
> > > > is
> > > > why we move there slowly and hopefully with proper deprecations. I
> was
> > > > hoping to do the update for community-templates since it's very easy
> > > > migration.
> > > >
> > > > If you think it's too complicated for users we could provide rake
> task or
> > > > migration. But please don't revert this.
> > >
> > > Yes, if you provided a migration it would be much better.  That doesn't
> > > really solve the problem with people using the foreman_templates plugin
> > > who will have those changes reverted, but I guess it's better than
> nothing.
> > >
> > > There's still dozens of other things allowed for @host in the Jail that
> > > aren't covered by these macros.   What's the plan for those?
> >
> > Whenever we have a chance, we should move from internal objects to
> macros. The
> > more macros we have the higher the chance is that we can keep templates
> > compatible.
>
> Sorry but I oppose this. Some internal objects are quite stable, in
> fact people rely upon them successfully - but we break the compatibility
> for apparent advantages that IMO are not worth the hassle.
>
> @host.operatingsystem, @host.architecture, etc... and @host.params, are
> very stable objects that can safely be called from templates and expect
> that will work for a long time. Not only our users do it. We also relied
> upon these internal objects for years.
>
> The first community templates PRs which are 4 years old used
> @host.params, @host.operatingsystem, etc.. Those are stable enough to
> not warrant writing a macro for them
>
> What we did on #16740 is basically renaming things around and deprecating
> params for macros that don't do anything but calling @host.params
> themselves.
>
> I would argue for the opposite way of thinking. When we change how
> these internal APIs work, we should deprecate them. If these APIs
> remain stable, don't change anything as it provides no value, and brings
> headaches and wastes time. Time wasted on writing this thread, on the
> people who write and review PRs in the project and associated ones
> (templates, the migration to new macros, fixing anything that might break,
> and adding more LOC which complicate our codebase).
>
> >
> > > I still think this provides more headaches than any benefits.
> >
> > I hope that following helps
> > * https://github.com/theforeman/community-templates/pull/343
> > * https://github.com/theforeman/foreman/pull/4187
> > * https://gist.github.com/ares/5435226ef0317613535101765404d3f5
> >
> > --
> > Marek
> >
> > >
> > >
> > > - Stephen
> > >
> > > > --
> > > > Marek
> > > >
> > > > --
> > > > 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+unsubscr...@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+unsubscr...@googlegroups.com.
> > For more options, visit https://groups.google.com/d/optout.
>
> --
> Daniel Lobato Garcia
>
> @dLobatog
> blog.daniellobato.me
> daniellobato.me
>
> GPG: http://keys.gnupg.net/pks/lookup?op=get=0x7A92D6DD38D6DE30
> Keybase: https://keybase.io/elobato
>
> --
> 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+unsubscr...@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 

Re: Re: [foreman-dev] Revert removal of @host.params for host_param

[Daniel Lobato Garcia]

On 01/12, Marek Hulán wrote:
 > >
> > > I strongly disagree that this does not have big benefits. Using internal
> > > Foreman objects in templates is a bad practice. It blocks us from
> > > improving
> > > our code. Therefore it's very important to build a DSL that users can use
> > > in templates and keep that compatible. We can then later change the
> > > implementation of host_param_true method without any templates changing.
> > > Think
> > > of this as a templating API.
> > >
> > > Another less significant benefit is that for plugins it's easier to
> > > wrap/alias
> > > the template method rather than manipulating something that's used
> > > internally in @host. Still not ideal but that should be solved by
> > > https://github.com/ theforeman/foreman/pull/3701
> > >
> > > Of course it will require users to migrate to new template helpers which
> > > is
> > > why we move there slowly and hopefully with proper deprecations. I was
> > > hoping to do the update for community-templates since it's very easy
> > > migration.
> > >
> > > If you think it's too complicated for users we could provide rake task or
> > > migration. But please don't revert this.
> >
> > Yes, if you provided a migration it would be much better.  That doesn't
> > really solve the problem with people using the foreman_templates plugin
> > who will have those changes reverted, but I guess it's better than nothing.
> >
> > There's still dozens of other things allowed for @host in the Jail that
> > aren't covered by these macros.   What's the plan for those?
>
> Whenever we have a chance, we should move from internal objects to macros. The
> more macros we have the higher the chance is that we can keep templates
> compatible.

Sorry but I oppose this. Some internal objects are quite stable, in
fact people rely upon them successfully - but we break the compatibility
for apparent advantages that IMO are not worth the hassle.

@host.operatingsystem, @host.architecture, etc... and @host.params, are
very stable objects that can safely be called from templates and expect
that will work for a long time. Not only our users do it. We also relied
upon these internal objects for years.

The first community templates PRs which are 4 years old used
@host.params, @host.operatingsystem, etc.. Those are stable enough to
not warrant writing a macro for them

What we did on #16740 is basically renaming things around and deprecating
params for macros that don't do anything but calling @host.params themselves.

I would argue for the opposite way of thinking. When we change how
these internal APIs work, we should deprecate them. If these APIs
remain stable, don't change anything as it provides no value, and brings
headaches and wastes time. Time wasted on writing this thread, on the
people who write and review PRs in the project and associated ones
(templates, the migration to new macros, fixing anything that might break,
and adding more LOC which complicate our codebase).

>
> > I still think this provides more headaches than any benefits.
>
> I hope that following helps
> * https://github.com/theforeman/community-templates/pull/343
> * https://github.com/theforeman/foreman/pull/4187
> * https://gist.github.com/ares/5435226ef0317613535101765404d3f5
>
> --
> Marek
>
> >
> >
> > - Stephen
> >
> > > --
> > > Marek
> > >
> > > --
> > > 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+unsubscr...@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+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

--
Daniel Lobato Garcia

@dLobatog
blog.daniellobato.me
daniellobato.me

GPG: http://keys.gnupg.net/pks/lookup?op=get=0x7A92D6DD38D6DE30
Keybase: https://keybase.io/elobato

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

Re: [foreman-dev] Revert removal of @host.params for host_param

[Marek Hulán]

On čtvrtek 12. ledna 2017 9:32:06 CET Stephen Benjamin wrote:
> - Original Message -
> 
> > From: "Marek Hulán" <mhu...@redhat.com>
> > To: foreman-dev@googlegroups.com
> > Sent: Thursday, January 12, 2017 2:59:39 AM
> > Subject: Re: [foreman-dev] Revert removal of @host.params for host_param
> > 
> > On středa 11. ledna 2017 14:22:53 CET Stephen Benjamin wrote:
> > > - Original Message -
> > > 
> > > > From: "Daniel Lobato Garcia" <elobat...@gmail.com>
> > > > To: foreman-dev@googlegroups.com
> > > > Sent: Wednesday, January 11, 2017 11:05:39 AM
> > > > Subject: [foreman-dev] Revert removal of @host.params for host_param
> > > > 
> > > > Hi foreman devs,
> > > > 
> > > > Just noticed today
> > > > https://github.com/theforeman/foreman/pull/3983/files
> > > > after some comments on IRC. What's the background behind this change?
> > > > 
> > > > As far as I can see, this merely moves
> > > > 
> > > > @host.param to host_param
> > > > @host.param_true? to host_param_true?
> > > > @host.param_false? to host_param_false?
> > > > @host.info to host_enc
> > > > 
> > > > without gaining anything from the change. This will force people to
> > > > change their templates (including our community templates) when the
> > > > deprecation is removed, and there's nothing to gain.
> > > > 
> > > > Does someone know what's the rationale behind this change? As it
> > > > stands
> > > > right now, I propose reverting that commit entirely to avoid
> > > > inflicting
> > > > that pain on users - which include many devs who maintain templates.
> > > > 
> > > > Best,
> > > 
> > > I know the macros look better, but it seems like a small gain for a
> > > lot of pain.  A lot of users use the existing methods in parameter
> > > values
> > > (ERB w/ safe mode off), and their own custom templates.
> > > 
> > > And the standard response to these kinds of complaints is "well, it's
> > > deprecated and users have enough time to change."  But I really just
> > > don't think that's sufficient, this is more change for the sake of
> > > change.
> > > 
> > > Also, the deprecation wasn't really smooth as it broke REX tests.
> > > 
> > >   http://ci.theforeman.org/job/test_plugin_matrix/2466/
> > > 
> > > - Stephen
> > 
> > Hello
> > 
> > I strongly disagree that this does not have big benefits. Using internal
> > Foreman objects in templates is a bad practice. It blocks us from
> > improving
> > our code. Therefore it's very important to build a DSL that users can use
> > in templates and keep that compatible. We can then later change the
> > implementation of host_param_true method without any templates changing.
> > Think
> > of this as a templating API.
> > 
> > Another less significant benefit is that for plugins it's easier to
> > wrap/alias
> > the template method rather than manipulating something that's used
> > internally in @host. Still not ideal but that should be solved by
> > https://github.com/ theforeman/foreman/pull/3701
> > 
> > Of course it will require users to migrate to new template helpers which
> > is
> > why we move there slowly and hopefully with proper deprecations. I was
> > hoping to do the update for community-templates since it's very easy
> > migration.
> > 
> > If you think it's too complicated for users we could provide rake task or
> > migration. But please don't revert this.
> 
> Yes, if you provided a migration it would be much better.  That doesn't
> really solve the problem with people using the foreman_templates plugin
> who will have those changes reverted, but I guess it's better than nothing.
> 
> There's still dozens of other things allowed for @host in the Jail that
> aren't covered by these macros.   What's the plan for those?

Whenever we have a chance, we should move from internal objects to macros. The 
more macros we have the higher the chance is that we can keep templates 
compatible.

> I still think this provides more headaches than any benefits.

I hope that following helps
* https://github.com/theforeman/community-templates/pull/343
* https://github.com/theforeman/foreman/pull/4187
* https://gist.github.com/ares/5435226ef0317613535101765404d3f5

--
Marek

> 
> 
> - Stephen
> 
> > --
> > Marek
> > 
> > --
> > 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+unsubscr...@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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [foreman-dev] Revert removal of @host.params for host_param

[Marek Hulán]

> Also, the deprecation wasn't really smooth as it broke REX tests.
>   http://ci.theforeman.org/job/test_plugin_matrix/2466/

One more note, REX was not broken by this, users were no affected. It's just 
the test that checks "no warning was reported". The test tests something more 
than it should. But in this case it's actually good that REX tests let us know 
that we should update REX code. We didn't have to and just fix the test, we 
didn't have to release new version with any fix either. It let us know early 
before the method was entirely disabled. I'm not sure what we could do better 
in this case.

--
Marek

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [foreman-dev] Revert removal of @host.params for host_param

[Marek Hulán]

On středa 11. ledna 2017 14:22:53 CET Stephen Benjamin wrote:
> - Original Message -
> 
> > From: "Daniel Lobato Garcia" 
> > To: foreman-dev@googlegroups.com
> > Sent: Wednesday, January 11, 2017 11:05:39 AM
> > Subject: [foreman-dev] Revert removal of @host.params for host_param
> > 
> > Hi foreman devs,
> > 
> > Just noticed today https://github.com/theforeman/foreman/pull/3983/files
> > after some comments on IRC. What's the background behind this change?
> > 
> > As far as I can see, this merely moves
> > 
> > @host.param to host_param
> > @host.param_true? to host_param_true?
> > @host.param_false? to host_param_false?
> > @host.info to host_enc
> > 
> > without gaining anything from the change. This will force people to
> > change their templates (including our community templates) when the
> > deprecation is removed, and there's nothing to gain.
> > 
> > Does someone know what's the rationale behind this change? As it stands
> > right now, I propose reverting that commit entirely to avoid inflicting
> > that pain on users - which include many devs who maintain templates.
> > 
> > Best,
> 
> I know the macros look better, but it seems like a small gain for a
> lot of pain.  A lot of users use the existing methods in parameter values
> (ERB w/ safe mode off), and their own custom templates.
> 
> And the standard response to these kinds of complaints is "well, it's
> deprecated and users have enough time to change."  But I really just
> don't think that's sufficient, this is more change for the sake of
> change.
> 
> Also, the deprecation wasn't really smooth as it broke REX tests.
>   http://ci.theforeman.org/job/test_plugin_matrix/2466/
> 
> 
> - Stephen

Hello

I strongly disagree that this does not have big benefits. Using internal 
Foreman objects in templates is a bad practice. It blocks us from improving 
our code. Therefore it's very important to build a DSL that users can use in 
templates and keep that compatible. We can then later change the 
implementation of host_param_true method without any templates changing. Think 
of this as a templating API.

Another less significant benefit is that for plugins it's easier to wrap/alias 
the template method rather than manipulating something that's used internally 
in @host. Still not ideal but that should be solved by https://github.com/
theforeman/foreman/pull/3701

Of course it will require users to migrate to new template helpers which is 
why we move there slowly and hopefully with proper deprecations. I was hoping 
to do the update for community-templates since it's very easy migration.

If you think it's too complicated for users we could provide rake task or 
migration. But please don't revert this.

--
Marek

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [foreman-dev] Revert removal of @host.params for host_param

[Stephen Benjamin]

- Original Message -
> From: "Daniel Lobato Garcia" 
> To: foreman-dev@googlegroups.com
> Sent: Wednesday, January 11, 2017 11:05:39 AM
> Subject: [foreman-dev] Revert removal of @host.params for host_param
> 
> Hi foreman devs,
> 
> Just noticed today https://github.com/theforeman/foreman/pull/3983/files
> after some comments on IRC. What's the background behind this change?
> 
> As far as I can see, this merely moves
> 
> @host.param to host_param
> @host.param_true? to host_param_true?
> @host.param_false? to host_param_false?
> @host.info to host_enc
> 
> without gaining anything from the change. This will force people to
> change their templates (including our community templates) when the
> deprecation is removed, and there's nothing to gain.
> 
> Does someone know what's the rationale behind this change? As it stands
> right now, I propose reverting that commit entirely to avoid inflicting
> that pain on users - which include many devs who maintain templates.
> 
> Best,
> 

I know the macros look better, but it seems like a small gain for a
lot of pain.  A lot of users use the existing methods in parameter values
(ERB w/ safe mode off), and their own custom templates.

And the standard response to these kinds of complaints is "well, it's
deprecated and users have enough time to change."  But I really just
don't think that's sufficient, this is more change for the sake of
change.

Also, the deprecation wasn't really smooth as it broke REX tests.
  http://ci.theforeman.org/job/test_plugin_matrix/2466/


- 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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.