Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Check secrets and ssl in production 2 #297

Merged
merged 4 commits into from Sep 4, 2015
Merged

Check secrets and ssl in production 2 #297

merged 4 commits into from Sep 4, 2015

Conversation

jordimassaguerpla
Copy link
Member

This PR adds additional checks and a nicer ui for error handling.

If you want to see how it would look in production, set config.consider_all_requests_local to false in config/environments/development.rb.

Here a couple of screenshots:

if database is not properly configured

http://ibin.co/2E3H61mok295

if secrets are not properly configured

http://ibin.co/2E3HNCRxMigl

@@ -0,0 +1,3 @@
# Place all the behaviors and hooks related to the matching controller here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this file. It's not used.

@mssola
Copy link
Collaborator

mssola commented Sep 1, 2015

Could you write a test to check that in a faulty configuration you would get redirected to the errors page ?

@jordimassaguerpla
Copy link
Member Author

@mssola : I've done the fixes you asked me. Please could you take a second look?
I am writing the tests and I'll add them in short.

@@ -42,5 +42,6 @@
put "toggle_admin", on: :member
end
end
match "(errors)/:status", to: "errors#show", constraints: { status: /\d{3}/ }, via: :all
Copy link
Collaborator

Choose a reason for hiding this comment

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

And now you can remove this match no ?

Copy link
Member Author

Choose a reason for hiding this comment

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

no i can't. This match is for routing the (errors) to the errors controller. By (errors) I mean 500, 400 errors, like when an exception is raised.

Copy link
Member

Choose a reason for hiding this comment

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

Does that include also the errors raised in other circumstances (like the 404 error trying to view a non existing team)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it does. Should we handle it differently? I don't know this case.

Copy link
Member

Choose a reason for hiding this comment

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

No, I just think it's great.

Implement an error controller to show custom messages when requirements
like ssl configuration or secrets configurations are not met. Also, set
up this controller as a custom error handler for showing up a custom
message if database is not accessible.

do not exit when running database migrations on production but
instead show the error page since any access to the database will
raise an exception.

This way we have a running application that states the configuration
errors.
@jordimassaguerpla
Copy link
Member Author

I've fixed the tests and rebased the branch. Thanks for your comments so far. Could you take another look?

<center><%= image_tag "layout/portus-error.png" %></center>
<% if @fix.value?(true) %>
<p>
<center><h1> Ops... something went wrong...</h1></center>
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the space before "Ops"

@flavio
Copy link
Member

flavio commented Sep 4, 2015

Please fix Rubocop's complains (rubocop -a)

@flavio
Copy link
Member

flavio commented Sep 4, 2015

LGTM, waiting for @mssola's feedback

use "begin ... ensure" to make sure the "after" block is run after
running the example
@jordimassaguerpla
Copy link
Member Author

@flavio : thanks. I've reviewed according to your changes. Let's wait for @mssola feedback.

mssola added a commit that referenced this pull request Sep 4, 2015
…in_production

Check secrets and ssl in production 2
@mssola mssola merged commit 7309057 into SUSE:master Sep 4, 2015
@jordimassaguerpla jordimassaguerpla deleted the check_secrets_and_ssl_in_production branch September 4, 2015 08:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants