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

Plugin removal functionality restored #420

Merged
merged 1 commit into from Jan 8, 2016

Conversation

Projects
None yet
2 participants
@dbaggott
Copy link
Contributor

commented Jan 4, 2016

  • reorganizes code for DRYness
  • install / remove actions both delegate to a new manage_plugin method

Note: this is derived from https://github.com/elastic/cookbook-elasticsearch/tree/martinb3/chef_proxy_support and
incorporates its proxy support changes which fixes #415

Fixes #418

@dbaggott

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2016

Investigating build failure...

@martinb3

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2016

The proxy functionality has been merged to master if you want to rebase against the latest.

@dbaggott

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2016

@martinb3 I'll rebase and clean up my commit history. I'm also fixing some style issues surrounding whitespace (shame on me for not running the tests!) and I'm currently looking into some non-trivial failures.

Do you have continuous builds running against master enabled? It looks to me like master doesn't pass the build but maybe I'm misunderstanding. Can you please advise?

[~/git/cookbook-elasticsearch (master)]$ bundle exec rake style
warning: parser/current is loading parser/ruby21, which recognizes
warning: 2.1.7-compliant syntax, but you are running 2.1.1.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
Running RuboCop...
/Users/dbaggott/git/cookbook-elasticsearch/libraries/helpers.rb
/Users/dbaggott/git/cookbook-elasticsearch/test/unit/spec/plugin_proxy_spec.rb
RuboCop failed!
[~/git/cookbook-elasticsearch (master)]$ rubocop -d
warning: parser/current is loading parser/ruby21, which recognizes
warning: 2.1.7-compliant syntax, but you are running 2.1.1.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
For /Users/dbaggott/git/cookbook-elasticsearch: configuration from /Users/dbaggott/git/cookbook-elasticsearch/.rubocop.yml
Default configuration from /opt/chefdk/embedded/lib/ruby/gems/2.1.0/gems/rubocop-0.35.1/config/default.yml
Inheriting configuration from /opt/chefdk/embedded/lib/ruby/gems/2.1.0/gems/rubocop-0.35.1/config/enabled.yml
Inheriting configuration from /opt/chefdk/embedded/lib/ruby/gems/2.1.0/gems/rubocop-0.35.1/config/disabled.yml
Inspecting 43 files
[removed Scanning output for clarity]

Offenses:

libraries/helpers.rb:3:3: C: Module has too many lines. [127/125]
  module Helpers
  ^^^^^^
libraries/helpers.rb:158:9: C: Do not prefix reader method names with get_.
    def get_configured_proxy
        ^^^^^^^^^^^^^^^^^^^^
libraries/helpers.rb:166:9: C: Do not prefix reader method names with get_.
    def get_java_proxy_arguments
        ^^^^^^^^^^^^^^^^^^^^^^^^
test/unit/spec/plugin_proxy_spec.rb:35:30: C: Prefer single-quoted strings when you don't need string interpolation or special symbols.
          expect(args).to eq("-DproxyHost=example.com -DproxyPort=80")
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
test/unit/spec/plugin_proxy_spec.rb:41:30: C: Prefer single-quoted strings when you don't need string interpolation or special symbols.
          expect(args).to eq("-DproxyHost=example.com -DproxyPort=8080")
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
test/unit/spec/plugin_proxy_spec.rb:47:30: C: Prefer single-quoted strings when you don't need string interpolation or special symbols.
          expect(args).to eq("-DproxyHost=example.com -DproxyPort=443")
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
test/unit/spec/plugin_proxy_spec.rb:53:30: C: Prefer single-quoted strings when you don't need string interpolation or special symbols.
          expect(args).to eq("-DproxyHost=example.com -DproxyPort=8080")
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
test/unit/spec/plugin_proxy_spec.rb:56:1: C: Extra blank line detected.
test/unit/spec/plugin_proxy_spec.rb:56:1: C: Extra empty line detected at block body end.

43 files inspected, 9 offenses detected
Finished in 0.104378 seconds
@martinb3

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2016

@dbaggott it was passing last I looked, but Rubocop also sometimes sneaks in new rules / refactorings. I've just pushed another commit to master to fix those rubocop warnings.

@dbaggott dbaggott force-pushed the dbaggott:issue-418-plugin-removal-broken branch from 535a0e9 to d00c750 Jan 5, 2016

@martinb3

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2016

Just pushed one more fix as well; it looks like the unit tests were resetting the chef proxy configuration to empty string, when it should have been nil. Chef tries to call methods on the proxy values at times I wasn't expecting, causing errors in places I wasn't expecting.

Plugin removal functionality restored
* reorganizes code for DRYness
* install / remove actions both delegate to a manage_plugin method

Fixes #418

@dbaggott dbaggott force-pushed the dbaggott:issue-418-plugin-removal-broken branch from d00c750 to 77b62f1 Jan 5, 2016

@dbaggott

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2016

@martinb3, great! I was just trying (unsuccessfully) to puzzle the host issue out.

I fixed my rubocop failures and rebased. Assuming the build is green (I think it should be), this is now ready for review. Let me know what you think. Fwiw, I'm happy to receive any style, etc nit-picking you might have... Don't hold back.

Thanks!

@martinb3 martinb3 merged commit 77b62f1 into elastic:master Jan 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@martinb3

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2016

Thanks @dbaggott!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.