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

Fixes #21467 - Updates to rubocop 0.51.0 #7036

Merged
merged 1 commit into from
Oct 26, 2017

Conversation

johnpmitsch
Copy link
Contributor

No description provided.

@theforeman-bot
Copy link

Issues: #21467

@johnpmitsch
Copy link
Contributor Author

@jlsherrill @akofink please review, (especially the new :dependent keys on the associations)

@jlsherrill
Copy link
Member

@johnpmitsch looks like something went wrong the URI change. The test failures all look related

@@ -115,11 +115,11 @@ def import_data(index_hosts = true)
pool_attributes[:pool_type] = pool_json["type"] if pool_json.key?("type")

if pool_attributes.key?(:multi_entitlement)
pool_attributes[:multi_entitlement] = pool_attributes[:multi_entitlement] == "yes" ? true : false
pool_attributes[:multi_entitlement] = pool_attributes[:multi_entitlement]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this still have == 'yes' on the end ?

end

if pool_attributes.key?(:virtual)
pool_attributes[:virt_only] = pool_attributes["virtual"] == 'true' ? true : false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this still have == 'true' on the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I had that changed back locally and guess I didn't push up!

@@ -3,7 +3,7 @@ require File.expand_path("../engine", File.dirname(__FILE__))
namespace :katello do
desc "Runs Rubocop style checker on Katello code"
task :rubocop do
system("bundle exec rubocop #{Katello::Engine.root}")
system("bundle exec rubocop -D #{Katello::Engine.root}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -37,13 +38,14 @@ module SmartProxyExtensions
has_many :lifecycle_environments,
:class_name => "Katello::KTEnvironment",
:through => :capsule_lifecycle_environments,
:source => :lifecycle_environment
:source => :lifecycle_environment,
:dependent => :nullify
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of these look fine, this one i'm still trying to figure out what :nullify would even do here. It seems like we'd want 'no action', since the has_many :capsule_lifecycle_environments would handle the destroy of the join object

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, this isn't reported through rubocop, and looking at rubocop/rubocop#4751 it is not expected to, so i think has_many :through associations should still not have a :dependent setting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it was just for the has_many :hostgroups below, updating now!

Copy link
Member

@akofink akofink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good. Just a few changes. It also looks as if you might need to update a few tests.

@@ -165,7 +165,7 @@ def publish_puppet_environment?

def promoted?
# if the view exists in more than 1 environment, it has been promoted
self.environments.length > 1 ? true : false
self.environments.length > 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.environments.many?

@johnpmitsch johnpmitsch force-pushed the rubocop_fixes branch 4 times, most recently from 9a7c66e to 9ae3f2c Compare October 26, 2017 13:08
@johnpmitsch
Copy link
Contributor Author

updated according to comments/irc discussion

@akofink
Copy link
Member

akofink commented Oct 26, 2017

@johnpmitsch code looks good. Please ping when tests are passing.

Copy link
Member

@akofink akofink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @johnpmitsch!

@jlsherrill
Copy link
Member

Going ahead and merging, thanks @johnpmitsch !

@jlsherrill jlsherrill merged commit d0ded38 into Katello:master Oct 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants