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

Jump to bottom

Fixes #26758 - unify renderer_error helper #6746

Merged
merged 1 commit into from Aug 25, 2019
Merged

Fixes #26758 - unify renderer_error helper #6746

merged 1 commit into from Aug 25, 2019

Conversation

lzap
Copy link
Member

@lzap lzap commented May 7, 2019

Preview actually throws argument error (3 vs 1).

@theforeman-bot
Copy link
Member

Issues: #26758

@lzap lzap requested a review from timogoebel May 16, 2019 14:15
@lzap
Copy link
Member Author

lzap commented May 16, 2019

Ho ho ho, tests failed. Hmmm.

@lzap
Copy link
Member Author

lzap commented Jul 22, 2019

Let's see if tests pass.

@lzap
Copy link
Member Author

lzap commented Aug 1, 2019

Thanks, added your commit as "refs". Let's see.

@timogoebel
Copy link
Member

Tests are still 🔴 :-(

@lzap
Copy link
Member Author

lzap commented Aug 14, 2019

Ok it was a mess, now it follows the same parameters as other controllers have: message and options hash.

timogoebel
timogoebel reviewed Aug 15, 2019
View reviewed changes
app/controllers/unattended_controller.rb Outdated
@@ -171,7 +171,11 @@ def verify_found_host
end

error = host_verifier.errors.first
render_error(error[:type], error[:message], error[:params])
if ipxe_request?
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't render_error already handle this path correctly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm refactoring of refactored code and you end up with this ;-)

@lzap
Copy link
Member Author

lzap commented Aug 15, 2019

Rebased, fixed missed test.

timogoebel
timogoebel reviewed Aug 15, 2019
View reviewed changes
app/controllers/unattended_controller.rb Outdated
if ipxe_request?
render_ipxe_message(message: message, status: options[:status] || :not_found)
else
super(message, options)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You can just call super here. No need to pass the parameters.

@lzap
Copy link
Member Author

lzap commented Aug 20, 2019

Rebased.

During RC testing I found that actually we have a bug in core which is fixed by this PR:

2019-08-20T11:41:56 [W|app|8cd14d86] Action failed
ArgumentError: wrong number of arguments (given 1, expected 3)
/usr/share/foreman/app/controllers/unattended_controller.rb:83:in `render_error'
/usr/share/foreman/app/controllers/concerns/foreman/controller/template_rendering.rb:19:in `rescue in safe_render'
/usr/share/foreman/app/controllers/concerns/foreman/controller/template_rendering.rb:15:in `safe_render'
/usr/share/foreman/app/controllers/concerns/foreman/controller/template_rendering.rb:9:in `render_template'
/usr/share/foreman/app/controllers/unattended_controller.rb:130:in `render_provisioning_template'

This appears only when there is a rendering error (which is the case for RC). I will propose this one for cherry pick.

@tbrisker
Copy link
Member

[test foreman]

@tbrisker tbrisker added this to the 1.23.0 milestone Aug 21, 2019
timogoebel
timogoebel approved these changes Aug 25, 2019
View reviewed changes
Copy link
Member

@timogoebel timogoebel left a comment

Choose a reason for hiding this comment

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

LGTM.

@tbrisker tbrisker merged commit 6bb5487 into theforeman:develop Aug 25, 2019
@tbrisker
Copy link
Member

1.23 - 11e8580

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

@timogoebel timogoebel timogoebel approved these changes

Assignees
No one assigned
Labels
Needs testing
Projects
None yet
Milestone
1.23.0
4 participants
@lzap @theforeman-bot @timogoebel @tbrisker