Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Fixes #16740 - Access host params through macro" #4219

Closed
wants to merge 1 commit into from

Conversation

dLobatog
Copy link
Member

This reverts commit 477b6b6.

The aim is to keep the @host.params API stable and not force users to
change anything, especially if they will get very little benefits.

Notice that existing tmeplates, the documentation, the wiki and the
"Help" tab of templates should be modified if this API changes too

There were alternative models to keep the existing '@host.' API proposed
in the foremna-dev thread linked below:

https://groups.google.com/forum/?fromgroups#!topic/foreman-dev/P364HozAYZA

@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • f93f4e0 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@mention-bot
Copy link

@dLobatog, thanks for your PR! By analyzing the history of the files in this pull request, we identified @lzap, @ares and @ohadlevy to be potential reviewers.

@ares
Copy link
Member

ares commented Jan 24, 2017

I suppose this didn't change your opinion - e0f04e1

@timogoebel
Copy link
Member

In my opinion, we should not revert this change. I think, this is a change that users won't mind. And they have enough time to change their templates (i'm not sure if we actually added the automatic migrations, yet).
If we do want to keep support for the "old" way, my suggestion would be to keep the new helper objects and change the community-templates and documentation to use them but not deprecate the "old" way. So users can choose too use whatever they like. The new objects do add some value, so users will probably adopt the change.

Just my two cents.

@bastilian
Copy link
Member

A possible compromise to this could be, having a Proxy/Decorator for models that are to be used in templates, and a TemplateMacroHelperMethods class that can provide macros by their name either as method names hard coded + define_method or via method_missing.

The Decorator holds the implementation, while the MacroHelperMethods class is mapper or lookup of methods called in the template renderer and can also be a guard of what is accessible from the Decorator.

Having this also gives a place to put methods we use controllers or views and don't really belong into view helpers or the model (Host::Common has a few candidates that I'd put there). This would help improve the overall code and provide a base with methods used in internal controllers and views and called as macros in templates.

My vote would be to either revert the change and implement the host params access via macro with the proposed pattern, or to leave as is and introduce the Decorator implementation in another refactor PR and implement the host_params macros based on that.

This reverts commit 477b6b6.

The aim is to keep the @host.params API stable and not force users to
change anything, especially if they will get very little benefits.

Notice that existing tmeplates, the documentation, the wiki and the
"Help" tab of templates should be modified if this API changes too

There were alternative models to keep the existing '@host.' API proposed
in the foremna-dev thread linked below:

https://groups.google.com/forum/?fromgroups#!topic/foreman-dev/P364HozAYZA
@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • 07f0733 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@mmoll
Copy link
Contributor

mmoll commented Mar 14, 2017

What's the status here?

@tbrisker tbrisker added the Reached an impasse Needs broader discussion label Apr 2, 2017
@dLobatog
Copy link
Member Author

Closing as I'm guessing we won't remove something that's released already

@dLobatog dLobatog closed this May 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants