Fixes #10635 - Formalize deprecation warning #2444

alongoldboim · 2015-06-09T10:39:49Z

I have added Foreman::Deprecation service which wraps ActiveSupport::Deprecation that we are currently using, formalizing the messages and enforces deprecation.
I have changed all the deprecations calls on the core to the new format and placed a default
two versions up (1.10.0) as a deadline(all deprecations with 1.10.0 will be deleted on it's release).

theforeman-bot · 2015-06-09T10:39:49Z

There were the following issues with the commit message:

dd175f7 must be in the format Fixes/refs #redmine_number - brief description.

Guidelines are available on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

domcleal · 2015-06-09T12:09:15Z

app/mailers/application_mailer.rb

@@ -15,7 +15,7 @@ def mail(headers = {}, &block)

  class GroupMail
    def initialize(emails)
-      ActiveSupport::Deprecation.warn 'GroupMail will be removed as mailers should not generate multiple messages, use MailNotification#deliver'
+      ForemanDeprecation.deprecation_warning("MailNotification","deliver","1.10.0","GroupMail will be removed as mailers should not generate multiple messages")


Same comment on all of these changes - code without spacing around arguments is hard to read, please include it as standard.

fixing.

2015-06-09 15:09 GMT+03:00 Dominic Cleal notifications@github.com:

In app/mailers/application_mailer.rb
#2444 (comment):

@@ -15,7 +15,7 @@ def mail(headers = {}, &block)

class GroupMail
def initialize(emails)

ActiveSupport::Deprecation.warn 'GroupMail will be removed as mailers should not generate multiple messages, use MailNotification#deliver'

ForemanDeprecation.deprecation_warning("MailNotification","deliver","1.10.0","GroupMail will be removed as mailers should not generate multiple messages")

Same comment on all of these changes - code without spacing around
arguments is hard to read, please include it as standard.

—
Reply to this email directly or view it on GitHub
https://github.com/theforeman/foreman/pull/2444/files#r32005809.

alongoldboim · 2015-06-10T08:10:38Z

app/services/foreman/deprecation.rb

+  class Deprecation
+    #aviliable options: controller, method, general info, callstack
+    def self.deprecation_warning(foreman_version_deadline, options={})
+      raise Foreman::Exception, _("Invalid version format, please enter in x.y.z") unless foreman_version_deadline.to_s.match(/\A^\d[.]\d+[.]\d$\z/)


@domcleal I tried using Gem:Version.correct? and it wasn't strict enough (e.g 1.2.e was accepted).
So I decided sticking with the Reg ex.

Hm okay, not sure that matters a lot.

The regexp now contains both \A and ^, ditto for EOL, just use one or the other.

domcleal · 2015-06-10T09:55:35Z

app/models/smart_proxy.rb

@@ -12,17 +12,17 @@ class SmartProxy < ActiveRecord::Base
  #TODO check if there is a way to look into the tftp_id too
  # maybe with a predefined sql
  has_and_belongs_to_many :features
-  has_many :subnets,                                          :foreign_key => 'dhcp_id'


Please don't re-format files like this, revert changes that aren't to do with this PR.

domcleal · 2015-06-10T09:58:20Z

Unit tests are failing due to ForemanDeprecation usage, and rubocop is also failing - possibly because of the reformatting, please revert those and run rubocop to see the remaining warnings.

domcleal · 2015-07-01T08:48:51Z

The author of your commit is set as @elobato according to GitHub, is that deliberate?

alongoldboim · 2015-07-01T09:06:51Z

It was by mistake, fixed it.

domcleal · 2015-07-01T10:18:46Z

Looks good. Just scanning for other deprecation warnings when rebased onto develop, I see:

app/controllers/api/v1/operatingsystems_controller.rb:91
config/initializers/deprecations.rb:4

Could you update those too please?

alongoldboim · 2015-07-01T10:57:36Z

Added them.
Thanks Dominic.

domcleal · 2015-07-01T11:13:24Z

config/initializers/deprecations.rb

@@ -2,6 +2,7 @@ module Deprecations
  def const_missing(const_name)
    return(super) unless const_name.to_s == 'ConfigTemplate'
    warn '`ConfigTemplate` has been deprecated. Use `ProvisioningTemplate` instead.'


And remove this to stop a duplicate warning?

sorry forgot :).

domcleal · 2015-07-02T11:15:09Z

Merged as 319d1ff for Foreman 1.9, thanks @alongoldboim.

theforeman-bot added Not yet reviewed Needs testing labels Jun 9, 2015

alongoldboim force-pushed the 10635 branch from dd175f7 to 58cdb62 Compare June 9, 2015 10:43

domcleal changed the title ~~Refactor #10635 - Formalize deprecation warning~~ Fixes #10635 - Formalize deprecation warning Jun 9, 2015

domcleal reviewed Jun 9, 2015
View reviewed changes

domcleal added Waiting on contributor and removed Not yet reviewed labels Jun 9, 2015

alongoldboim force-pushed the 10635 branch from 58cdb62 to 5178360 Compare June 10, 2015 08:04

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jun 10, 2015

alongoldboim reviewed Jun 10, 2015
View reviewed changes

alongoldboim force-pushed the 10635 branch from 5178360 to b9cd742 Compare June 10, 2015 08:14

domcleal reviewed Jun 10, 2015
View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels Jun 10, 2015

alongoldboim force-pushed the 10635 branch from b0faae5 to bef2bce Compare July 1, 2015 09:05

domcleal added Waiting on contributor and removed Needs re-review labels Jul 1, 2015

alongoldboim force-pushed the 10635 branch from bef2bce to a7dfeed Compare July 1, 2015 09:13

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jul 1, 2015

domcleal added Waiting on contributor and removed Needs re-review labels Jul 1, 2015

alongoldboim force-pushed the 10635 branch from a7dfeed to 31b85df Compare July 1, 2015 10:42

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jul 1, 2015

domcleal reviewed Jul 1, 2015
View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels Jul 1, 2015

Fixes #10635 - Formalize deprecation warning

f19c482

alongoldboim force-pushed the 10635 branch from 31b85df to f19c482 Compare July 1, 2015 11:21

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jul 1, 2015

domcleal added Ready to merge and removed Needs re-review Needs testing labels Jul 2, 2015

domcleal closed this Jul 2, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #10635 - Formalize deprecation warning #2444

Fixes #10635 - Formalize deprecation warning #2444

alongoldboim commented Jun 9, 2015

theforeman-bot commented Jun 9, 2015

domcleal Jun 9, 2015

alongoldboim Jun 9, 2015

alongoldboim Jun 10, 2015

domcleal Jun 10, 2015

domcleal Jun 10, 2015

domcleal commented Jun 10, 2015

domcleal commented Jul 1, 2015

alongoldboim commented Jul 1, 2015

domcleal commented Jul 1, 2015

alongoldboim commented Jul 1, 2015

domcleal Jul 1, 2015

alongoldboim Jul 1, 2015

domcleal commented Jul 2, 2015

Fixes #10635 - Formalize deprecation warning #2444

Fixes #10635 - Formalize deprecation warning #2444

Conversation

alongoldboim commented Jun 9, 2015

theforeman-bot commented Jun 9, 2015

domcleal Jun 9, 2015

Choose a reason for hiding this comment

alongoldboim Jun 9, 2015

Choose a reason for hiding this comment

alongoldboim Jun 10, 2015

Choose a reason for hiding this comment

domcleal Jun 10, 2015

Choose a reason for hiding this comment

domcleal Jun 10, 2015

Choose a reason for hiding this comment

domcleal commented Jun 10, 2015

domcleal commented Jul 1, 2015

alongoldboim commented Jul 1, 2015

domcleal commented Jul 1, 2015

alongoldboim commented Jul 1, 2015

domcleal Jul 1, 2015

Choose a reason for hiding this comment

alongoldboim Jul 1, 2015

Choose a reason for hiding this comment

domcleal commented Jul 2, 2015