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 #23145 - handle strong params for compute attributes on failure #5412

Merged
merged 1 commit into from
Apr 12, 2018

Conversation

iNecas
Copy link
Member

@iNecas iNecas commented Apr 6, 2018

See Redmine for description of the issue.

@theforeman-bot
Copy link
Member

Issues: #23145

@iNecas
Copy link
Member Author

iNecas commented Apr 6, 2018

Screenshot of the failure I'm talking about:

foreman-provision-fail

See Redmine for description of the issue.
Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

Code makes sense, didn't test though.
I hope removing the rescue won't lead to other exceptions blowing up, but in any case swallowing them silently is a Bad Idea &tm; and we should handle them properly.

@tbrisker
Copy link
Member

@iNecas - What happens if an actual error is encountered when talking to the compute resource?
@xprazak2 - I think we don't need to block branching on this, but i guess it would be good to get into the RC so we test this works properly

@iNecas
Copy link
Member Author

iNecas commented Apr 11, 2018

It depends on when the call is actually made, during the orchestration, the exception will be captured, as any other exception would be. When rendering, it's harder to capture, but that's an issue that is already in other places: just think about any other call that we don't rescue from (such as listing templates for select box or list of networks for NIC).

Btw. next thing I would like to do is to remove test_connection call from new_vm call, as it incredibly slows down the whole process, and I don't see any point of having it there. But keeping that outside of this PR

@iNecas
Copy link
Member Author

iNecas commented Apr 12, 2018

Tests are 💚, this should be ready for merge.

Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

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

Tested, fixes the problem
review

Thanks @iNecas, merging

@ares ares merged commit d7a9d13 into theforeman:develop Apr 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants