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

[BUG] Fallback mode should respect the connection errors #67

Closed
wants to merge 1 commit into from

Conversation

maschwenk
Copy link

When builds experience a 500 from the connection, Knapsack should not throw an error, but rather use fallback mode. A successful connection should both be successful AND have no errors.

ArgumentError: {"status"=>"500", "error"=>"Internal Server Error"}
--
  | /usr/src/app/vendor/bundle/ruby/2.3.0/gems/knapsack_pro-0.53.0/lib/knapsack_pro/allocator.rb:14:in `test_file_paths'
  | /usr/src/app/vendor/bundle/ruby/2.3.0/gems/knapsack_pro-0.53.0/lib/knapsack_pro/runners/base_runner.rb:14:in `test_file_paths'
  | /usr/src/app/vendor/bundle/ruby/2.3.0/gems/knapsack_pro-0.53.0/lib/knapsack_pro/runners/base_runner.rb:26:in `test_files_to_execute_exist?'
  | /usr/src/app/vendor/bundle/ruby/2.3.0/gems/knapsack_pro-0.53.0/lib/knapsack_pro/runners/rspec_runner.rb:10:in `run'

Maybe an alternative would be to warn when there are errors but still fall back? Either way this is blocking builds in production for us.

@ArturT
Copy link
Member

ArturT commented Sep 28, 2018

Hi

Sorry for the issue. I had problem with DB backup and the server exceeded the memory that caused problem.
I applied fix and I'm going to work on improvement in knapsack_pro gem to handle this scenario in Fallback Mode so tests could be run without API as expected (this time the fallback mode does not started :( and I'm going to fix it).

I'm working on the improvement and I will let you know when new knapsack_pro version will be available.

I'm also planning to upgrade to bigger servers but I will announce this later (upgrade should be during one of future weekends and the fallback mode should work then to ensure tests can be run).

@maschwenk
Copy link
Author

I understand that, but if the response contains any error key in response hash the existing code looks like it ignores it and raises a run-time exception. Doesn't my PR fix that issue?

@ArturT
Copy link
Member

ArturT commented Sep 28, 2018

I will need to adjust this code because there are scenarios when API returns 4xx error when you for instance not provided git commit hash then we want to raise error and show to user that there is missing info about commit hash. Probably I should not raise ArgumentError when error API returned 5xx so this way the fallback mode should start.

I'm going to work on preparing a proper fix but I need to do some local tests first.

FYI The Knapsack Pro API is working right now.

@ArturT
Copy link
Member

ArturT commented Sep 28, 2018

A new version of knapsack_pro gem has been released with better detection when to run tests in fallback mode in case of API issues. Please update.

https://github.com/KnapsackPro/knapsack_pro-ruby/blob/master/CHANGELOG.md#100

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.

None yet

2 participants