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 #64 - Authenticate request with WS repository management API key #81

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

lacek
Copy link

@lacek lacek commented Feb 1, 2016

This addresses #64 partially, as the API is mandatory but not optional as @fnkr mentioned in the issue.

@fnkr
Copy link

fnkr commented Feb 1, 2016

@lacek Making it mandatory would be event better. I updated my issue. If you add "Fixes #64" to the description of your PR, it will close it automatically, as soon as this is merged.

@lacek lacek changed the title Authenticate request with WS repository management API key Fixes #64 - Authenticate request with WS repository management API key Feb 1, 2016
@koppen
Copy link
Owner

koppen commented Feb 1, 2016

Cool, thanks for your contribution - and thanks to @fnkr for bringing it up. I like this change, but I have a few comments and questions.

@lacek
Copy link
Author

lacek commented Feb 1, 2016

@koppen By Redmine's default Setting.sys_api_enabled is false. Thus this update breaks the plugin for user running with default settings, until they enable the sys_api_enabled and generate an API key.
I am not sure if this is the expected behavior. If not I would suggest creating separate configurations for this plugin.

I am a newbie to Ruby and Redmine Plugin development. Do you have any hint to run the test cases?
I got the following error when trying to run them under Redmine 3.2.0 with Ruby 2.1.0:

redmine@23316d7deffc:~/redmine$ bundle exec rake redmine:plugins:test:functionals NAME=redmine_github_hook
/usr/bin/ruby2.1 -I"lib:test" -I"/home/redmine/redmine/vendor/bundle/ruby/2.1.0/gems/rake-10.5.0/lib" "/home/redmine/redmine/vendor/bundle/ruby/2.1.0/gems/rake-10.5.0/lib/rake/rake_test_loader.rb" "plugins/redmine_github_hook/test/functional/**/*_test.rb" 
MiniTest::Unit::TestCase is now Minitest::Test. From /usr/lib/ruby/2.1.0/test/unit/testcase.rb:8:in `<module:Unit>'
/usr/lib/ruby/2.1.0/test/unit.rb:676:in `<class:Runner>': undefined method `_run_suite' for class `Test::Unit::Runner' (NameError)
    from /usr/lib/ruby/2.1.0/test/unit.rb:261:in `<module:Unit>'
    from /usr/lib/ruby/2.1.0/test/unit.rb:15:in `<module:Test>'
    from /usr/lib/ruby/2.1.0/test/unit.rb:7:in `<top (required)>'
    from /home/redmine/redmine/vendor/bundle/ruby/2.1.0/gems/activesupport-4.2.5/lib/active_support/dependencies.rb:274:in `require'
    from /home/redmine/redmine/vendor/bundle/ruby/2.1.0/gems/activesupport-4.2.5/lib/active_support/dependencies.rb:274:in `block in require'
    from /home/redmine/redmine/vendor/bundle/ruby/2.1.0/gems/activesupport-4.2.5/lib/active_support/dependencies.rb:240:in `load_dependency'
    from /home/redmine/redmine/vendor/bundle/ruby/2.1.0/gems/activesupport-4.2.5/lib/active_support/dependencies.rb:274:in `require'
    from /home/redmine/redmine/plugins/redmine_github_hook/test/functional/github_hook_controller_test.rb:3:in `<top (required)>'
    from /home/redmine/redmine/vendor/bundle/ruby/2.1.0/gems/rake-10.5.0/lib/rake/rake_test_loader.rb:10:in `require'
    from /home/redmine/redmine/vendor/bundle/ruby/2.1.0/gems/rake-10.5.0/lib/rake/rake_test_loader.rb:10:in `block (2 levels) in <main>'
    from /home/redmine/redmine/vendor/bundle/ruby/2.1.0/gems/rake-10.5.0/lib/rake/rake_test_loader.rb:9:in `each'
    from /home/redmine/redmine/vendor/bundle/ruby/2.1.0/gems/rake-10.5.0/lib/rake/rake_test_loader.rb:9:in `block in <main>'
    from /home/redmine/redmine/vendor/bundle/ruby/2.1.0/gems/rake-10.5.0/lib/rake/rake_test_loader.rb:4:in `select'
    from /home/redmine/redmine/vendor/bundle/ruby/2.1.0/gems/rake-10.5.0/lib/rake/rake_test_loader.rb:4:in `<main>'
rake aborted!
Command failed with status (1): [ruby -I"lib:test" -I"/home/redmine/redmine/vendor/bundle/ruby/2.1.0/gems/rake-10.5.0/lib" "/home/redmine/redmine/vendor/bundle/ruby/2.1.0/gems/rake-10.5.0/lib/rake/rake_test_loader.rb" "plugins/redmine_github_hook/test/functional/**/*_test.rb" ]

Do I need to rollback to older version of Redmine and Ruby for testing?

@koppen
Copy link
Owner

koppen commented Feb 2, 2016

By Redmine's default Setting.sys_api_enabled is false. Thus this update breaks the plugin for user running with default settings, until they enable the sys_api_enabled and generate an API key.

I am not sure if this is the expected behavior. If not I would suggest creating separate configurations for this plugin.

I would very much like to not suddenly break the integration for everybody if it could be avoided. But I guess we could release a major version for this, and document it thoroughly.

Would it make sense to simply check for the existence of the API key and only verify it if it is present?

Do I need to rollback to older version of Redmine and Ruby for testing?

I hope not, but I admit that it has been a while since a last did any real development on the plugin, but I'm pretty sure it has never been particularly easy to run the test suite. Let me look into and I'll get back to you.

@koppen
Copy link
Owner

koppen commented Feb 2, 2016

Alright, looks like the tests were still expecting Test::Unit. I've updated master with a switch to MiniTest, so you should be good to go now.

And yes, rake redmine:plugins:test is what I use to run my tests as well.

@lacek
Copy link
Author

lacek commented Feb 3, 2016

Would it make sense to simply check for the existence of the API key and only verify it if it is present?

Yes, this do provide flexibility for those who do not want API authentication.
I've made the change, fixed and added test cases. Thanks for your fix, all tests pass now.

edit: I've messed file permission up after running test cases, rolled back and re-pushed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants