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

Add url config to redis #99

Merged
merged 4 commits into from Apr 17, 2018

Conversation

Projects
None yet
4 participants
@pedantix
Copy link
Member

pedantix commented Apr 4, 2018

No description provided.

Shaun Hubbard
@pedantix
Copy link
Member Author

pedantix left a comment

Modified Tests/RedisTests/RedisTests.swift because I was having intermittent failures, Running FLUSHALL seems to have fixed it, even though this is not reliable because of its async nature.

@bre7

This comment has been minimized.

Copy link

bre7 commented Apr 5, 2018

I was going to submit a new PR but could you run git ls-files -ci --exclude-standard -z | xargs -0 git rm --cached to re-apply the .gitignore and remove the .DS_Store files ?

@pedantix

This comment has been minimized.

Copy link
Member Author

pedantix commented Apr 17, 2018

@bre7 did I add a .DS_store file in here? I am not seeing it. Sorry for the late reply on this I try to be better than that.

@tanner0101
Copy link
Member

tanner0101 left a comment

LGTM overall, but I think the usage of URL should be limited to convenience init on RedisClientConfig, it shouldn't become the actual config object.

@@ -8,18 +8,22 @@ public final class RedisDatabase: Database {
public typealias Connection = RedisClient

/// This client's configuration.
public let config: RedisClientConfig
public let url: URL

This comment has been minimized.

@tanner0101

tanner0101 Apr 17, 2018

Member

this should still be RedisClientConfig

Shaun Hubbard and others added some commits Apr 17, 2018

Shaun Hubbard
Shaun Hubbard

@pedantix pedantix merged commit b0ad379 into vapor:master Apr 17, 2018

2 checks passed

ci/circleci: linux Your tests passed on CircleCI!
Details
ci/circleci: swiftlint Your tests passed on CircleCI!
Details

@pedantix pedantix deleted the pedantix:add-url-config-to-redis branch Apr 17, 2018

@kjcolley7

This comment has been minimized.

Copy link

kjcolley7 commented Apr 17, 2018

@pedantix: Is this change intended to remove the initializer for RedisClientConfig where you give it hostname, password, etc? Just broke my project...

/vapor/Sources/App/configure.swift:71:20: error: cannot invoke initializer for type 'RedisClientConfig' with an argument list of type '(hostname: String, password: String?)'
        let redisConfig = RedisClientConfig(
                          ^
/vapor/Sources/App/configure.swift:71:20: note: overloads for 'RedisClientConfig' exist with these partially matching parameter lists: (url: URL), (), (from: Decoder)
        let redisConfig = RedisClientConfig(
@@ -68,9 +69,32 @@ public struct RedisClientConfig: Codable {
public var password: String?

/// Create a new `RedisClientConfig`
public init(hostname: String = "localhost", port: Int = 6379, password: String? = nil) {

This comment has been minimized.

@kjcolley7

This comment has been minimized.

@pedantix

pedantix Apr 17, 2018

Author Member

@kjcolley7 I don't want to break anyones project... but this is pre release :( so I am going to have to ask you to adjust as we finalize API sorry. would you be willing to try somthing like

let hostname = "localhost"
let port = 5679
let connectionString = "redis://(hostname):(port)"
let url = URL(string: connectionString)
and using that URL to see if it restores your projects functionality?

This comment has been minimized.

@kjcolley7

kjcolley7 Apr 17, 2018

That works, thanks!

This comment has been minimized.

@pedantix

pedantix Apr 17, 2018

Author Member

thanks for using it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment