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

Strip Heroku-style usernames from config url #58

Merged
merged 1 commit into from
May 10, 2016

Conversation

acorncom
Copy link
Contributor

Fixes #36

@acorncom
Copy link
Contributor Author

@lukemelia Thoughts here?

},

_stripUsernameFromConfigUrl: function(configUrl) {

Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick, can you remove this?

@ghedamat
Copy link
Contributor

@acorncom thanks for this, LGTM aside from the tiny comment

just catching up on the issue here,

basically we always need to specify username separately for that to work then?

/cc @achambers @lukemelia

@lukemelia
Copy link
Contributor

LGTM @acorncom. I'm a little worried that some setups may use the username somehow, but I guess we can introduce a config flag later if that turns out to be the case. Merge when you're happy @ghedamat

@ghedamat ghedamat self-assigned this Apr 27, 2016
@acorncom
Copy link
Contributor Author

@ghedamat Re: always needing to specify the username, from what I've been seeing it looks like the underlying then-redis library swaps the Redis DB into the username spot (more details here: #51 (comment)). Seems like it's not properly following the Redis URL spec.

At this point, I'm not sure if we want to merge this PR or work on fixing the upstream library (assuming they're willing to do it). But I'm not a huge fan of using non-spec compliant URLs ;-)

Thoughts?

@acorncom
Copy link
Contributor Author

@ghedamat Spacing issue fixed too ;-)

@joshsmith
Copy link

joshsmith commented Apr 29, 2016

I've been lurking on this issue for awhile, and I would vote for attempting to fix the upstream library.

EDIT: Of course, this is easy for me to say when I'm inventing work for other people.

@ghedamat ghedamat merged commit f151829 into ember-cli-deploy:master May 10, 2016
@ghedamat
Copy link
Contributor

thanks @acorncom and sorry for the delay!

@ghedamat
Copy link
Contributor

@lukemelia maybe this is worth a minor version bump and a mention in the changelog?

@lukemelia
Copy link
Contributor

@ghedamat agreed.

@acorncom acorncom deleted the strip-heroku-username branch May 10, 2016 18:27
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

4 participants