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

Allow https #156

Merged
merged 3 commits into from
Nov 25, 2019
Merged

Allow https #156

merged 3 commits into from
Nov 25, 2019

Conversation

javierg
Copy link
Member

@javierg javierg commented Nov 25, 2019

Set use_ssl based on protocol option under couchdb.yml

Copy link
Contributor

@ErickFabian ErickFabian left a comment

Choose a reason for hiding this comment

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

:shipit:

@javierg javierg merged commit 13909bd into 3.0 Nov 25, 2019
@javierg javierg deleted the allow_https branch November 25, 2019 21:39
http.request(req)
end
end

def secure?
env['protocol'] == SECURE_PROTOCOL
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering if we should drop the protocol part of this since it doesn't look like we use that in the underlying connection and instead just use a use_ssl: true type flag in the config.

Copy link
Member Author

Choose a reason for hiding this comment

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

protocol is used on https://github.com/amco/dolly/blob/3.0/lib/dolly/configuration.rb#L13 to set the base_uri for the connection.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but it could be hard coded as constants and the correct one used based on the use_ssl flag. It is fine either way, just seems we could create one less typo issue and not have to worry about a string comparison. Was just a thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, will raise an issue to do so

Copy link
Member Author

Choose a reason for hiding this comment

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

I just need to check as this will make current config invalid, as not many projects have Dolly3, should be fine, but there are some on development already using it.

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