Fixes #7233 - Drop Ruby 1.8.7 and revert Ruby 1.8.7 specifics #1784

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@shlomizadok
Member

shlomizadok commented Sep 22, 2014

  • rake test passed
  • rails server loaded and function correctly
  • Ubuntu 12.04 packaging blocker issue
  • drop 1.8.7 tests on Jenkins
@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Sep 22, 2014

Contributor

As a reminder, please do not merge this until the Ubuntu 12.04 packaging blocker issue is at least investigated, and preferably on track or complete.

Contributor

domcleal commented Sep 22, 2014

As a reminder, please do not merge this until the Ubuntu 12.04 packaging blocker issue is at least investigated, and preferably on track or complete.

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Sep 22, 2014

Member

@domcleal 👍 - Would love to help with Ubuntu 12.04 should you need.

Member

shlomizadok commented Sep 22, 2014

@domcleal 👍 - Would love to help with Ubuntu 12.04 should you need.

@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Sep 22, 2014

Contributor

http://projects.theforeman.org/issues/7227 has details, it's not being worked on AFAIK.

Contributor

domcleal commented Sep 22, 2014

http://projects.theforeman.org/issues/7227 has details, it's not being worked on AFAIK.

@dLobatog

This comment has been minimized.

Show comment
Hide comment
@dLobatog

dLobatog Oct 15, 2014

Member

Any progress? Should we mark this as reached an impasse?

Member

dLobatog commented Oct 15, 2014

Any progress? Should we mark this as reached an impasse?

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Oct 15, 2014

Member

@elobato -- did not reach impasse. Waiting for issue #7227 to be completed (gr8 job @domcleal !). Once it's done, we can remove 1.8.7 tests from Jenkins and merge this PR

Member

shlomizadok commented Oct 15, 2014

@elobato -- did not reach impasse. Waiting for issue #7227 to be completed (gr8 job @domcleal !). Once it's done, we can remove 1.8.7 tests from Jenkins and merge this PR

@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Oct 17, 2014

Contributor

Jenkins is no longer testing Ruby 1.8.

Before this is merged (and there's no rush), I'd like to verify the gem versions you're changing to are all going to work with our RPM packages, I'll need to do a round of updates. I'll try and do this in the next week or so, once the last of this 1.8 packaging changes are out of the way.

I'd also like to question why some of the gems you're pinning very tightly (~> patch version), e.g. net-ldap, ldap_fluff or rabl, but in other cases you're leaving entirely open, e.g. apipie-rails or fast_gettext. I'd prefer we stuck with one scheme or the other - my personal feeling is we should generally pin only to prevent incompatible changes (e.g. semver major version).

I'm also concerned that you've removed minimum versions from a number of specifications (e.g. bundler.d/gettext.rb), which are probably there for good reason - i.e. that we depend on a feature introduced in that version.

Contributor

domcleal commented Oct 17, 2014

Jenkins is no longer testing Ruby 1.8.

Before this is merged (and there's no rush), I'd like to verify the gem versions you're changing to are all going to work with our RPM packages, I'll need to do a round of updates. I'll try and do this in the next week or so, once the last of this 1.8 packaging changes are out of the way.

I'd also like to question why some of the gems you're pinning very tightly (~> patch version), e.g. net-ldap, ldap_fluff or rabl, but in other cases you're leaving entirely open, e.g. apipie-rails or fast_gettext. I'd prefer we stuck with one scheme or the other - my personal feeling is we should generally pin only to prevent incompatible changes (e.g. semver major version).

I'm also concerned that you've removed minimum versions from a number of specifications (e.g. bundler.d/gettext.rb), which are probably there for good reason - i.e. that we depend on a feature introduced in that version.

@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Oct 17, 2014

Contributor

Oh, and lastly you should consider reverting the rubocop style guidelines that require hash rockets for Ruby 1.8. (Maybe any others if we have them, e.g. lambdas.)

Contributor

domcleal commented Oct 17, 2014

Oh, and lastly you should consider reverting the rubocop style guidelines that require hash rockets for Ruby 1.8. (Maybe any others if we have them, e.g. lambdas.)

@dLobatog

This comment has been minimized.

Show comment
Hide comment
@dLobatog

dLobatog Oct 20, 2014

Member

@domcleal I thought about it for a while (removing the style HashRockets thingy), in my opinion as long as we are consistent with that it's ok to keep it. I like the hash literal syntax better myself but I doubt the effort is worth it as we have a consistent hash syntax usage already.

Member

dLobatog commented Oct 20, 2014

@domcleal I thought about it for a while (removing the style HashRockets thingy), in my opinion as long as we are consistent with that it's ok to keep it. I like the hash literal syntax better myself but I doubt the effort is worth it as we have a consistent hash syntax usage already.

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Oct 20, 2014

Member

Which is why I have removed the enforce on hash_rockets

Member

shlomizadok commented Oct 20, 2014

Which is why I have removed the enforce on hash_rockets

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Oct 20, 2014

Member

[test]

Member

shlomizadok commented Oct 20, 2014

[test]

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Oct 23, 2014

Member

@domcleal -- will appreciate another look at this. Thanks :)

Member

shlomizadok commented Oct 23, 2014

@domcleal -- will appreciate another look at this. Thanks :)

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Oct 26, 2014

Member

rebased and re[test]. @ohadlevy @domcleal @elobato -- second look?

Member

shlomizadok commented Oct 26, 2014

rebased and re[test]. @ohadlevy @domcleal @elobato -- second look?

@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Oct 27, 2014

Contributor

Please stop pinging me, I'll get to it when I can. You've made this quite complex, so it's going to take some time to re-check what's been changed on each of those dependencies.

Contributor

domcleal commented Oct 27, 2014

Please stop pinging me, I'll get to it when I can. You've made this quite complex, so it's going to take some time to re-check what's been changed on each of those dependencies.

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Oct 27, 2014

Member

@shlomizadok it might be easier not to squash, so people can see the diff since the last time they reviewed.

also, note, tests are failing.

Member

ohadlevy commented Oct 27, 2014

@shlomizadok it might be easier not to squash, so people can see the diff since the last time they reviewed.

also, note, tests are failing.

@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Nov 21, 2014

Contributor

I'm going to compare this against our available RPM versions, which means we may need to reduce some of the minimum dependencies to match what's available. I'll update packages that we carry where possible, but for stuff in (RH)SCL, we should use what's already there instead of updating it. Some updates we'll get later when we move to the Rails 4 collections.

Although we don't currently validate version numbers from the gemfile against the RPM versions (due to bundler_ext's default behaviour), I'd prefer that our version numbers match.

Contributor

domcleal commented Nov 21, 2014

I'm going to compare this against our available RPM versions, which means we may need to reduce some of the minimum dependencies to match what's available. I'll update packages that we carry where possible, but for stuff in (RH)SCL, we should use what's already there instead of updating it. Some updates we'll get later when we move to the Rails 4 collections.

Although we don't currently validate version numbers from the gemfile against the RPM versions (due to bundler_ext's default behaviour), I'd prefer that our version numbers match.

Gemfile
-gem 'ldap_fluff', '>= 0.3.0', '< 1.0.0'
-gem 'uuidtools'
-gem "apipie-rails", "~> 0.2.5"
+gem 'json', '~> 1.8'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

SCL has 1.5.5, I'd suggest ~> 1.5

@domcleal

domcleal Nov 21, 2014

Contributor

SCL has 1.5.5, I'd suggest ~> 1.5

Gemfile
-gem 'uuidtools'
-gem "apipie-rails", "~> 0.2.5"
+gem 'json', '~> 1.8'
+gem 'rest-client', '~> 1.7', :require => 'rest_client'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

F19 and our builds are 1.6.7, we should permit this

@domcleal

domcleal Nov 21, 2014

Contributor

F19 and our builds are 1.6.7, we should permit this

Gemfile
+gem 'rest-client', '~> 1.7', :require => 'rest_client'
+gem 'audited-activerecord', '3.0.0'
+gem 'will_paginate', '~> 3.0'
+gem 'ancestry', '~> 2.1'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

F19 and our builds are 2.0.0, we should permit this

@domcleal

domcleal Nov 21, 2014

Contributor

F19 and our builds are 2.0.0, we should permit this

Gemfile
+gem 'audited-activerecord', '3.0.0'
+gem 'will_paginate', '~> 3.0'
+gem 'ancestry', '~> 2.1'
+gem 'scoped_search'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

Keep the version spec here, as 3.x may break API?

@domcleal

domcleal Nov 21, 2014

Contributor

Keep the version spec here, as 3.x may break API?

Gemfile
+gem 'will_paginate', '~> 3.0'
+gem 'ancestry', '~> 2.1'
+gem 'scoped_search'
+gem 'net-ldap', '~> 0.8'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

Can be removed, ldap_fluff depends on it, we no longer use it directly.

@domcleal

domcleal Nov 21, 2014

Contributor

Can be removed, ldap_fluff depends on it, we no longer use it directly.

Gemfile
+gem 'scoped_search'
+gem 'net-ldap', '~> 0.8'
+gem 'ldap_fluff', '~> 0.3'
+gem 'uuidtools', '~> 2.1'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

This is fine, but we can also remove it: http://projects.theforeman.org/issues/8461

@domcleal

domcleal Nov 21, 2014

Contributor

This is fine, but we can also remove it: http://projects.theforeman.org/issues/8461

Gemfile
-gem 'secure_headers', '~> 1.3.3'
+gem 'secure_headers', '~> 1.3'
+gem 'safemode', '~> 1.2'
+gem 'ruby_parser', '3.0.4'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

Why a specific version?

@domcleal

domcleal Nov 21, 2014

Contributor

Why a specific version?

This comment has been minimized.

@shlomizadok

shlomizadok Nov 21, 2014

Member

Higher versions raises Safemode::SecurityError on provision templates and unattended_controller

@shlomizadok

shlomizadok Nov 21, 2014

Member

Higher versions raises Safemode::SecurityError on provision templates and unattended_controller

This comment has been minimized.

@domcleal

domcleal Nov 24, 2014

Contributor

I can't reproduce this with ruby2ruby 2.1.3, ruby_parser 3.6.3 when previewing templates.

@domcleal

domcleal Nov 24, 2014

Contributor

I can't reproduce this with ruby2ruby 2.1.3, ruby_parser 3.6.3 when previewing templates.

This comment has been minimized.

@domcleal

domcleal Nov 24, 2014

Contributor

Also our packages and F19 both have 3.1.1

@domcleal

domcleal Nov 24, 2014

Contributor

Also our packages and F19 both have 3.1.1

bundler.d/facter.rb
@@ -1,3 +1,3 @@
group :facter do
- gem 'facter'
+ gem 'facter', '~> 2.2'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

Please don't specify a version for Facter

@domcleal

domcleal Nov 21, 2014

Contributor

Please don't specify a version for Facter

bundler.d/assets.rb
- gem 'execjs', '< 2.1.0'
- gem "jquery-rails", "2.0.3"
+ gem 'sass-rails', '~> 3.2'
+ gem 'uglifier', '~> 2.5'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

F19 and SCL have 1.3.0 and 1.2.6, maybe stick with the existing version req.

@domcleal

domcleal Nov 21, 2014

Contributor

F19 and SCL have 1.3.0 and 1.2.6, maybe stick with the existing version req.

bundler.d/assets.rb
- gem "jquery-rails", "2.0.3"
+ gem 'sass-rails', '~> 3.2'
+ gem 'uglifier', '~> 2.5'
+ gem 'execjs', '~> 2.2'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

F19 and SCL have 1.4.0

@domcleal

domcleal Nov 21, 2014

Contributor

F19 and SCL have 1.4.0

bundler.d/assets.rb
- gem 'jquery_pwstrength_bootstrap'
+ gem 'therubyracer', '0.11.3', :require => 'v8'
+ gem 'bootstrap-sass', '3.0.3.0'
+ gem 'spice-html5-rails', '0.1.4'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

Should this be less specific?

@domcleal

domcleal Nov 21, 2014

Contributor

Should this be less specific?

bundler.d/assets.rb
+ gem 'spice-html5-rails', '0.1.4'
+ gem 'flot-rails', '0.0.3'
+ gem 'quiet_assets', '~> 1.0'
+ gem 'gettext_i18n_rails_js', '0.0.8'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

less specific?

@domcleal

domcleal Nov 21, 2014

Contributor

less specific?

bundler.d/assets.rb
+ gem 'flot-rails', '0.0.3'
+ gem 'quiet_assets', '~> 1.0'
+ gem 'gettext_i18n_rails_js', '0.0.8'
+ gem 'gettext', '~> 3.1', :require => false

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

Action on me to update our package from 2.3.7

@domcleal

domcleal Nov 21, 2014

Contributor

Action on me to update our package from 2.3.7

bundler.d/assets.rb
+ gem 'quiet_assets', '~> 1.0'
+ gem 'gettext_i18n_rails_js', '0.0.8'
+ gem 'gettext', '~> 3.1', :require => false
+ gem 'locale', '~> 2.1'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

Action on me to update our package from 2.0.9

@domcleal

domcleal Nov 21, 2014

Contributor

Action on me to update our package from 2.0.9

bundler.d/assets.rb
+ gem 'gettext_i18n_rails_js', '0.0.8'
+ gem 'gettext', '~> 3.1', :require => false
+ gem 'locale', '~> 2.1'
+ gem 'multi-select-rails', '0.9.10'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

This is a possible downgrade, maybe less specific?

Action on me, our package is 0.9.10 but could be .12.

@domcleal

domcleal Nov 21, 2014

Contributor

This is a possible downgrade, maybe less specific?

Action on me, our package is 0.9.10 but could be .12.

- gem 'wirb'
- gem 'hirb-unicode'
- gem 'awesome_print', :require => 'ap'
+ gem 'wirb', '~> 1.0'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

[Fixed] Action on me, our package is 0.4.2

@domcleal

domcleal Nov 21, 2014

Contributor

[Fixed] Action on me, our package is 0.4.2

bundler.d/console.rb
- gem 'hirb-unicode'
- gem 'awesome_print', :require => 'ap'
+ gem 'wirb', '~> 1.0'
+ gem 'hirb-unicode', '0.0.5'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

less specific perhaps?

@domcleal

domcleal Nov 21, 2014

Contributor

less specific perhaps?

bundler.d/console.rb
- gem 'awesome_print', :require => 'ap'
+ gem 'wirb', '~> 1.0'
+ gem 'hirb-unicode', '0.0.5'
+ gem 'awesome_print', '~> 1.2', :require => 'ap'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

F19 and we have 1.0.2, so this should be reduced.

I'm not sure we need to worry much about pinning the console group, it's not really production usage.

@domcleal

domcleal Nov 21, 2014

Contributor

F19 and we have 1.0.2, so this should be reduced.

I'm not sure we need to worry much about pinning the console group, it's not really production usage.

bundler.d/development.rb
# for generating foreign key migrations
- gem 'immigrant'
+ gem 'immigrant', '0.1.8'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

less specific?

@domcleal

domcleal Nov 21, 2014

Contributor

less specific?

bundler.d/development.rb
- gem 'immigrant'
+ gem 'immigrant', '0.1.8'
+
+ gem 'pry', '~> 0.10'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

I wouldn't bother pinning this

@domcleal

domcleal Nov 21, 2014

Contributor

I wouldn't bother pinning this

bundler.d/fog.rb
- gem 'unf'
+ gem 'fog', '1.24.0'
+ gem 'fog-core', '1.24.0'
+ gem 'unf', '0.1.4'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

less specific perhaps, especially as we don't actively use it? We have 0.1.3 in our packages.

@domcleal

domcleal Nov 21, 2014

Contributor

less specific perhaps, especially as we don't actively use it? We have 0.1.3 in our packages.

bundler.d/test.rb
+ gem 'spork', '~> 0.9'
+ gem 'factory_girl_rails', '~> 1.7', :require => false
+ gem 'oj', '~> 2.10'
+ gem 'rubocop-checkstyle_formatter', '0.1.1'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

too specific?

@domcleal

domcleal Nov 21, 2014

Contributor

too specific?

bundler.d/vmware.rb
@@ -1,3 +1,3 @@
group :vmware do
- gem "rbvmomi", '~> 1.6.0'
+ gem 'rbvmomi', '~> 1.6'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

rebase :)

@domcleal

domcleal Nov 21, 2014

Contributor

rebase :)

@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Nov 21, 2014

Contributor

Thanks @shlomizadok, looks good. Some of the pins were specific to a single version, what's the rationale for that? I think I left a comment on each one.

Contributor

domcleal commented Nov 21, 2014

Thanks @shlomizadok, looks good. Some of the pins were specific to a single version, what's the rationale for that? I think I left a comment on each one.

@theforeman-bot

This comment has been minimized.

Show comment
Hide comment
@theforeman-bot

theforeman-bot Nov 21, 2014

Member

There were the following issues with the commit message:

  • 2521489 must be in the format Fixes/refs #redmine_number - brief description.
  • 9a3db4d 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

Member

theforeman-bot commented Nov 21, 2014

There were the following issues with the commit message:

  • 2521489 must be in the format Fixes/refs #redmine_number - brief description.
  • 9a3db4d 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

@@ -3,7 +3,7 @@
sequence(:name) {|n| "notification#{n}"}
default_interval "Daily"
mailer "HostMailer"
- method "test_mail"

This comment has been minimized.

@shlomizadok

shlomizadok Nov 21, 2014

Member

This broke FactoryGirl

@shlomizadok

shlomizadok Nov 21, 2014

Member

This broke FactoryGirl

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Nov 21, 2014

Member

@domcleal , Thanks for the review and for testing with the packaged versions.
Places where gem versions were pinned to a specific version are gems that higher versions broke our tests / code (e.g., higher gem than gem 'ruby_parser', '3.0.4' will break our code).
While rebasing and upgrading, I have encountered three issues:

  1. Had to replace all the factories who started with Factory to FactoryGirl (deprecated)
  2. Had to replace old FactoryGirl after_create & after_build methods to after(:create) & after(:build) (deprecated)
  3. In MailNotification class we have an attribute method - while trying to a assign it in FactoryGirl, FactoryGirl will break (method not found) - Had to create an alias on that attribute and use the alias at the factory.
Member

shlomizadok commented Nov 21, 2014

@domcleal , Thanks for the review and for testing with the packaged versions.
Places where gem versions were pinned to a specific version are gems that higher versions broke our tests / code (e.g., higher gem than gem 'ruby_parser', '3.0.4' will break our code).
While rebasing and upgrading, I have encountered three issues:

  1. Had to replace all the factories who started with Factory to FactoryGirl (deprecated)
  2. Had to replace old FactoryGirl after_create & after_build methods to after(:create) & after(:build) (deprecated)
  3. In MailNotification class we have an attribute method - while trying to a assign it in FactoryGirl, FactoryGirl will break (method not found) - Had to create an alias on that attribute and use the alias at the factory.
@theforeman-bot

This comment has been minimized.

Show comment
Hide comment
@theforeman-bot

theforeman-bot Nov 21, 2014

Member

There were the following issues with the commit message:

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

Member

theforeman-bot commented Nov 21, 2014

There were the following issues with the commit message:

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

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Nov 21, 2014

Contributor

I guess the Katello failures are due to the FactoryGirl update - I didn't realise it'd be quite that disruptive, but I think it's a good idea, thanks for doing it.

I'll just check that we can build these newer package versions before committing, but it looks good so far.

Contributor

domcleal commented Nov 21, 2014

I guess the Katello failures are due to the FactoryGirl update - I didn't realise it'd be quite that disruptive, but I think it's a good idea, thanks for doing it.

I'll just check that we can build these newer package versions before committing, but it looks good so far.

-gem 'scoped_search', '>= 2.7.0', '< 3.0.0'
-gem 'net-ldap'
-gem 'ldap_fluff', '>= 0.3.0', '< 1.0.0'
-gem 'uuidtools'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

[Done] You need to update lib/foreman.rb to use SecureRandom if you're dropping the dep entirely: http://projects.theforeman.org/issues/8461

It's installed by luck right now, as google-api-client depends on it.

@domcleal

domcleal Nov 21, 2014

Contributor

[Done] You need to update lib/foreman.rb to use SecureRandom if you're dropping the dep entirely: http://projects.theforeman.org/issues/8461

It's installed by luck right now, as google-api-client depends on it.

bundler.d/development.rb
# for generating foreign key migrations
- gem 'immigrant'
+ gem 'immigrant', '~> 0.1.8'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

~> 0.1, or just leave the version off again? I'm less sure that pinning development/test dependencies is important, especially to this precision.

@domcleal

domcleal Nov 21, 2014

Contributor

~> 0.1, or just leave the version off again? I'm less sure that pinning development/test dependencies is important, especially to this precision.

bundler.d/assets.rb
+ gem 'spice-html5-rails', '~> 0.1.4'
+ gem 'flot-rails', '0.0.3'
+ gem 'quiet_assets', '~> 1.0'
+ gem 'gettext_i18n_rails_js', '0.0.8'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

My last comment's been lost, why this strict pinning? I think it could be more loose.

@domcleal

domcleal Nov 21, 2014

Contributor

My last comment's been lost, why this strict pinning? I think it could be more loose.

bundler.d/console.rb
- gem 'awesome_print', :require => 'ap'
+ gem 'wirb', '~> 1.0'
+ gem 'hirb-unicode', '~> 0.0.5'
+ gem 'awesome_print', '1.0.2', :require => 'ap'

This comment has been minimized.

@domcleal

domcleal Nov 21, 2014

Contributor

~> 1.0?

@domcleal

domcleal Nov 21, 2014

Contributor

~> 1.0?

@theforeman-bot

This comment has been minimized.

Show comment
Hide comment
@theforeman-bot

theforeman-bot Nov 21, 2014

Member

There were the following issues with the commit message:

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

Member

theforeman-bot commented Nov 21, 2014

There were the following issues with the commit message:

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

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Nov 21, 2014

Member

Fixes for Foreman side. Katello fixes will be applied on a separate PR

Member

shlomizadok commented Nov 21, 2014

Fixes for Foreman side. Katello fixes will be applied on a separate PR

bundler.d/assets.rb
+ gem 'spice-html5-rails', '~> 0.1.4'
+ gem 'flot-rails', '0.0.3'
+ gem 'quiet_assets', '~> 1.0'
+ gem 'gettext_i18n_rails_js', '~> 0.0'

This comment has been minimized.

@domcleal

domcleal Nov 24, 2014

Contributor

>= 0.0.8 was specifically required, you should probably state both.

@domcleal

domcleal Nov 24, 2014

Contributor

>= 0.0.8 was specifically required, you should probably state both.

@@ -1,7 +1,7 @@
module Foreman
# generate a UUID
def self.uuid
- UUIDTools::UUID.random_create.to_s
+ SecureRandom.uuid.to_s

This comment has been minimized.

@domcleal

domcleal Nov 24, 2014

Contributor

[Done] Nitpick, add require 'securerandom' to this file

@domcleal

domcleal Nov 24, 2014

Contributor

[Done] Nitpick, add require 'securerandom' to this file

+ gem 'flot-rails', '0.0.3'
+ gem 'quiet_assets', '~> 1.0'
+ gem 'gettext_i18n_rails_js', '~> 0.0', '>= 0.0.8'
+ gem 'gettext', '~> 3.1', :require => false

This comment has been minimized.

@domcleal

domcleal Nov 24, 2014

Contributor

[Fixed] Action on me, we package 2.3.7.

@domcleal

domcleal Nov 24, 2014

Contributor

[Fixed] Action on me, we package 2.3.7.

Gemfile
+gem 'ancestry', '~> 2.0'
+gem 'scoped_search', '~> 2.7'
+gem 'ldap_fluff', '~> 0.3'
+gem 'apipie-rails', '~> 0.2'

This comment has been minimized.

@domcleal

domcleal Nov 24, 2014

Contributor

0.2.5 minimum is needed here I think, we rely on those features

@domcleal

domcleal Nov 24, 2014

Contributor

0.2.5 minimum is needed here I think, we rely on those features

Gemfile
+gem 'ancestry', '~> 2.0'
+gem 'scoped_search', '~> 2.7'
+gem 'ldap_fluff', '~> 0.3'
+gem 'apipie-rails', '~> 0.2'
gem 'rabl', '>= 0.7.5', '<= 0.9.0'

This comment has been minimized.

@domcleal

domcleal Nov 24, 2014

Contributor

Could you file a refactor ticket please so we can update this later too? I realise it caused some breakage.

@domcleal

domcleal Nov 24, 2014

Contributor

Could you file a refactor ticket please so we can update this later too? I realise it caused some breakage.

- gem 'fast_gettext', '>=0.8.0'
- gem 'gettext_i18n_rails', '~> 0.10'
- gem 'gettext_i18n_rails_js', '>= 0.0.8'
- gem 'i18n_data', '>= 0.2.6', :require => 'i18n_data'

This comment has been minimized.

@domcleal

domcleal Nov 24, 2014

Contributor

Action on me, need to remove this package.

@domcleal

domcleal Nov 24, 2014

Contributor

Action on me, need to remove this package.

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
bundler.d/assets.rb
+ gem 'quiet_assets', '~> 1.0'
+ gem 'gettext_i18n_rails_js', '~> 0.0', '>= 0.0.8'
+ gem 'gettext', '~> 3.1', :require => false
+ gem 'locale', '~> 2.1'

This comment has been minimized.

@domcleal

domcleal Nov 24, 2014

Contributor

This will need to be 2.0, it looks like we can't rebase the RPMs, as they're also in use in Hammer, which is running on 1.8 on EL6.

@domcleal

domcleal Nov 24, 2014

Contributor

This will need to be 2.0, it looks like we can't rebase the RPMs, as they're also in use in Hammer, which is running on 1.8 on EL6.

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Nov 25, 2014

Member

rebased and upgraded Rabl gem (with ruby 2.1 in mind), pinned ruby_parser to 3.1.1

Member

shlomizadok commented Nov 25, 2014

rebased and upgraded Rabl gem (with ruby 2.1 in mind), pinned ruby_parser to 3.1.1

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Nov 25, 2014

Member

Tested again with Rabl (~> 0.11)and ruby_parser 3.1.1
Katello: http://ci.theforeman.org/job/test_katello_core/9677/
Core/Cop: http://ci.theforeman.org/job/test_develop_pull_request/9692/
🎉

Member

shlomizadok commented Nov 25, 2014

Tested again with Rabl (~> 0.11)and ruby_parser 3.1.1
Katello: http://ci.theforeman.org/job/test_katello_core/9677/
Core/Cop: http://ci.theforeman.org/job/test_develop_pull_request/9692/
🎉

bundler.d/gce.rb
- gem "google-api-client", :require => "google/api_client"
- gem 'sshkey'
+ gem 'google-api-client', '~> 0.7', :require => 'google/api_client'
+ gem 'sshkey', '~> 1.6'

This comment has been minimized.

@domcleal

domcleal Nov 25, 2014

Contributor

Could you relax this to 1.3 please? F19 ships 1.3.1.

@domcleal

domcleal Nov 25, 2014

Contributor

Could you relax this to 1.3 please? F19 ships 1.3.1.

@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Nov 25, 2014

Contributor

Thanks, we're there! I've done full builds of the RPMs and only hit that sshkey issue during repoclosure, got a couple of deps to build.

Please fix that last item, then squash it all into one commit - don't worry about the separate one for uuidtools.

Contributor

domcleal commented Nov 25, 2014

Thanks, we're there! I've done full builds of the RPMs and only hit that sshkey issue during repoclosure, got a couple of deps to build.

Please fix that last item, then squash it all into one commit - don't worry about the separate one for uuidtools.

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Nov 25, 2014

Member

rebased, squashed, gce ~> 1.3.
@domcleal Thank you so much for this.
(ping 👍 )

Member

shlomizadok commented Nov 25, 2014

rebased, squashed, gce ~> 1.3.
@domcleal Thank you so much for this.
(ping 👍 )

@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Nov 25, 2014

Contributor

Thanks @shlomizadok, merged as 51a8843 for Foreman 1.8. Nice to see new, shiny things we can use.

Contributor

domcleal commented Nov 25, 2014

Thanks @shlomizadok, merged as 51a8843 for Foreman 1.8. Nice to see new, shiny things we can use.

@domcleal domcleal closed this Nov 25, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment