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

multiple domain support #259

Merged
merged 5 commits into from Dec 1, 2017
Merged

multiple domain support #259

merged 5 commits into from Dec 1, 2017

Conversation

alexey-voronenko
Copy link
Contributor

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 3c4c466 on multiple-domain into d7e806d on master.

1 similar comment
@coveralls
Copy link

coveralls commented Nov 29, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 3c4c466 on multiple-domain into d7e806d on master.

lib/howitzer.rb Outdated
@@ -37,12 +37,13 @@ def mailgun_idle_timeout
# @example returns url without auth
# app_uri.origin

def self.app_uri
def self.app_uri(name = nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

unit tests are missing

Copy link
Contributor

Choose a reason for hiding this comment

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

If user forgot to create the settings, howitzer needs to show Warning: You used host, but forgot to define settings, please add following settings:...

Copy link
Contributor

Choose a reason for hiding this comment

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

Change log should be update

Copy link
Contributor

Choose a reason for hiding this comment

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

code documentation should be extended

Copy link
Contributor

@romikoops romikoops left a comment

Choose a reason for hiding this comment

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

See my comments

@coveralls
Copy link

coveralls commented Nov 30, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling a76e6be on multiple-domain into d7e806d on master.

CHANGELOG.md Outdated
@@ -2,6 +2,7 @@

### New Features
- Cucumber rake tasks minor updates
- Add multiple domain support
Copy link
Contributor

Choose a reason for hiding this comment

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

We have issue for it. It means, we need to provide link to original issue

lib/howitzer.rb Outdated
@@ -36,14 +36,25 @@ def mailgun_idle_timeout
# app_uri.site
# @example returns url without auth
# app_uri.origin
# @example returns url for custom host(should be specified in config)
Copy link
Contributor

Choose a reason for hiding this comment

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

# @example returns url for custom host
# app_uri(:example).site

lib/howitzer.rb Outdated
)
rescue NoMethodError
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like settings are validating in SexySettings automatically. Do we really need the message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If setting is not present it throws exception like this:

undefined method `example_app_base_auth_login' for Howitzer:Module (NoMethodError)

With this rescue message becomes more clear:

config is missing for custom host example, please add to config:
example_app_base_auth_login
example_app_base_auth_pass
example_app_host
example_app_protocol (NoMethodError)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c3e08a9 on multiple-domain into d7e806d on master.

1 similar comment
@coveralls
Copy link

coveralls commented Dec 1, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling c3e08a9 on multiple-domain into d7e806d on master.

@coveralls
Copy link

coveralls commented Dec 1, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 21eee08 on multiple-domain into d7e806d on master.

@romikoops romikoops merged commit 1cb2aef into master Dec 1, 2017
@romikoops romikoops deleted the multiple-domain branch December 1, 2017 23:54
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

3 participants