Revert removal of @host.params for host_param

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.

··· 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


Marek

> 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

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


Later,
Lukas @lzap Zapletal

Sorry to relive the topic but
https://github.com/theforeman/foreman/pull/4219 didn't get in,
and it's time to update the documentation for 1.15 including deprecations.

Since reverting the change is probably something not to be done during RC,
I wonder what to do.
The options are:

  1. Keep the code as-is - which forces people to change their templates for
    1.17 for little benefit imo
  2. Support both options - this would just require to remove the
    deprecations for the next RC which is not a problem
  3. Support both options and reconsider to remove the helpers?

Let me know what you think, I lean towards 2 or 3 as I explained in here
previously.

2017年1月11日水曜日 17時05分42秒 UTC+1 Daniel Lobato:

··· > > 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&search=0x7A92D6DD38D6DE30 > Keybase: https://keybase.io/elobato >

> From: "Marek Hulán" <mhulan@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
>
> >
> > > From: "Daniel Lobato Garcia" <elobatocs@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?

I still think this provides more headaches than any benefits.

  • Stephen
··· ----- Original Message ----- > On středa 11. ledna 2017 14:22:53 CET Stephen Benjamin wrote: > > ----- Original Message -----


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

  1. or 2., if we release 1.15 with those macros, I think 3. is not an option.
··· On pondělí 17. dubna 2017 11:22:07 CEST Daniel Lobato wrote: > Sorry to relive the topic but > https://github.com/theforeman/foreman/pull/4219 didn't get in, > and it's time to update the documentation for 1.15 including deprecations. > > Since reverting the change is probably something not to be done during RC, > I wonder what to do. > The options are: > > 1. Keep the code as-is - which forces people to change their templates for > 1.17 for little benefit imo > 2. Support both options - this would just require to remove the > deprecations for the next RC which is not a problem > 3. Support both options and reconsider to remove the helpers? > > Let me know what you think, I lean towards 2 or 3 as I explained in here > previously.


Marek

2017年1月11日水曜日 17時05分42秒 UTC+1 Daniel Lobato:

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,

@dLobatog
blog.daniellobato.me
daniellobato.me

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

>
> > From: "Marek Hulán" <mhulan@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
> >
> > >
> > > > From: "Daniel Lobato Garcia" <elobatocs@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

··· On čtvrtek 12. ledna 2017 9:32:06 CET Stephen Benjamin wrote: > ----- Original Message ----- > > On středa 11. ledna 2017 14:22:53 CET Stephen Benjamin wrote: > > > ----- Original Message -----


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

Since it seems there wasn't an agreement on reverting this made in time for
1.15, I'd say we should support both for now and reconsider in the future
pending a rewrite of the template engine using a proxy object as we
discussed.

··· On Tue, Apr 18, 2017 at 10:15 AM, Marek Hulán wrote:

On pondělí 17. dubna 2017 11:22:07 CEST Daniel Lobato wrote:

Sorry to relive the topic but
https://github.com/theforeman/foreman/pull/4219 didn’t get in,
and it’s time to update the documentation for 1.15 including
deprecations.

Since reverting the change is probably something not to be done during
RC,
I wonder what to do.
The options are:

  1. Keep the code as-is - which forces people to change their templates
    for
    1.17 for little benefit imo
  2. Support both options - this would just require to remove the
    deprecations for the next RC which is not a problem
  3. Support both options and reconsider to remove the helpers?

Let me know what you think, I lean towards 2 or 3 as I explained in here
previously.

  1. or 2., if we release 1.15 with those macros, I think 3. is not an
    option.


Marek

2017年1月11日水曜日 17時05分42秒 UTC+1 Daniel Lobato:

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,

@dLobatog
blog.daniellobato.me
daniellobato.me

GPG: http://keys.gnupg.net/pks/lookup?op=get&search=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+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Have a nice day,
Tomer Brisker
Red Hat Engineering

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

··· On 01/12, Marek Hulán wrote:

I still think this provides more headaches than any benefits.

I hope that following helps


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


Daniel Lobato Garcia

@dLobatog
blog.daniellobato.me
daniellobato.me

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

+1, supporting both seems like the way forward.

Greg

··· On Wed, 2017-04-19 at 14:10 +0300, Tomer Brisker wrote: > Since it seems there wasn't an agreement on reverting this made in > time for 1.15, I'd say we should support both for now and reconsider > in the future pending a rewrite of the template engine using a proxy > object as we discussed.

At the very least support both. This is a point release and this is a
pretty major change for a non-major release.

As per the comment from Ewoud the bulk of people who will use this use it
in ERB and templates. The templates being probably the easiest to 'fix'.
Personally, keeping this permanently and proxying to me seems like the
right thing to do. This functionality has existed for years and is very
heavily documented all over the place.

··· On Wednesday, April 19, 2017 at 12:39:40 PM UTC-4, Greg Sutcliffe wrote: > > On Wed, 2017-04-19 at 14:10 +0300, Tomer Brisker wrote: > > Since it seems there wasn't an agreement on reverting this made in > > time for 1.15, I'd say we should support both for now and reconsider > > in the future pending a rewrite of the template engine using a proxy > > object as we discussed. > > +1, supporting both seems like the way forward. > > Greg >

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


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


Daniel Lobato Garcia

@dLobatog
blog.daniellobato.me
daniellobato.me

GPG: http://keys.gnupg.net/pks/lookup?op=get&search=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+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Right now 1.15 is released. I decided not to document the deprecation as
the consensus seems to
maintain both ways of addressing this.
The deprecations are still going to show up in the logs, so I suggest
someone submits a PR to remove
the deprecations and cherry-pick it for 1.15.1

2017年4月20日木曜日 10時19分19秒 UTC+9 Andrew Schofield:

··· > > At the very least support both. This is a point release and this is a > pretty major change for a non-major release. > > As per the comment from Ewoud the bulk of people who will use this use it > in ERB and templates. The templates being probably the easiest to 'fix'. > Personally, keeping this permanently and proxying to me seems like the > right thing to do. This functionality has existed for years and is very > heavily documented all over the place. > > On Wednesday, April 19, 2017 at 12:39:40 PM UTC-4, Greg Sutcliffe wrote: >> >> On Wed, 2017-04-19 at 14:10 +0300, Tomer Brisker wrote: >> > Since it seems there wasn't an agreement on reverting this made in >> > time for 1.15, I'd say we should support both for now and reconsider >> > in the future pending a rewrite of the template engine using a proxy >> > object as we discussed. >> >> +1, supporting both seems like the way forward. >> >> Greg >> >

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” sokeeffe@redhat.com 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 elobatocs@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


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


Daniel Lobato Garcia

@dLobatog
blog.daniellobato.me
daniellobato.me

GPG: http://keys.gnupg.net/pks/lookup?op=get&search=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+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.

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” sokeeffe@redhat.com 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 < >> elobatocs@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


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


Daniel Lobato Garcia

@dLobatog
blog.daniellobato.me
daniellobato.me

GPG: http://keys.gnupg.net/pks/lookup?op=get&search=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+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.


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.

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

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

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


Have a nice day,
Tomer Brisker
Red Hat Engineering

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

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

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