Detect Defaults for Laravel Scanner: DB, Redis #3090
Conversation
…tting the DatabaseDesired and SkipDatabase values based on detection
…er subsequent found
scanner/laravel.go
Outdated
|
|
||
| // Set up Regex to match | ||
| // -not commented out, with DB_CONNECTION | ||
| dbReg := regexp.MustCompile("DB_CONNECTION *= *[a-zA-Z]+") |
There was a problem hiding this comment.
this regex matches # DB_CONNECTION = ..., does it? (comment line)
There was a problem hiding this comment.
Nice catch!! The regex above is incorrect! It ain't gonna ignore commented out DB_CONNECTIONS, thanks for the eagle eyes on this!
Fixed this by only accepting whitespace before the DB_CONNECTION and reject variations with other characters before the string, ultimately ignoring lines where comment characters are added!
…whitespaces before DB_CONNECTION
Hi @dangra! Thank you for taking the time check my PR, I appreciate it! I've fixed the issue you've pointed above, and I'm eager to know how I can improve it, so when you're free, please do list some notes I can use to improve my PR |
scanner/laravel.go
Outdated
| /* | ||
| Determine the default db | ||
| Determine whether redis connection is desired | ||
| returns ( db, redis, skipDB ) | ||
| */ |
There was a problem hiding this comment.
See how to document Functions https://tip.golang.org/doc/comment#func
There was a problem hiding this comment.
nice! this helps, let me work on it, thank you!
scanner/laravel.go
Outdated
| // Set up Regex to match | ||
| // -not commented out, with DB_CONNECTION | ||
| dbReg := regexp.MustCompile("^ *DB_CONNECTION *= *[a-zA-Z]+") | ||
| // -not commented out with redis keyword | ||
| redisReg := regexp.MustCompile("^[^#]*redis") |
There was a problem hiding this comment.
static regular patterns (and its regex.MustCompile() calls) are ideal as global variables as they can catch regex syntax errors earlier. More details at https://stackoverflow.com/a/72185182
scanner/laravel.go
Outdated
| if strings.Contains(text, "mysql") { | ||
| db = DatabaseKindMySQL | ||
| skipDb = false | ||
| } else if strings.Contains(text, "pgsql") { | ||
| db = DatabaseKindPostgres | ||
| skipDb = false | ||
| } else if strings.Contains(text, "sqlite") { | ||
| db = DatabaseKindSqlite | ||
| skipDb = false |
There was a problem hiding this comment.
text contains is very loose matching, a better approach would URI parsing. See https://gobyexample.com/url-parsing
There was a problem hiding this comment.
Laravel's config isn't a connection string that'll work with parseURL, - in this case DB_CONNECTION is one of mysql, pgsql, sqlite, sqlsrv and in rarer cases, something custom. It's not a full DB connection string tho.
(There is a lesser-used DATABASE_URL env var that Laravel can use, but it's rarer to see used).
There was a problem hiding this comment.
@fideloper TIL. The PR is fine as it is. My comment is a nitpick of things that can be improved.
In this case it can use regex group matching to extract the value from the line and compare more strictly by using switch/case Go idiom.
And it is good that you mention it, support for DATABASE_URL would be good to have too, not a must ofc.
There was a problem hiding this comment.
And it is good that you mention it, support for DATABASE_URL would be good to have too, not a must ofc.
Agree!
There was a problem hiding this comment.
Team thanks!! I added the DATABSE_URL case in the regex to check connections from!
…le for laravel apps
Change Summary
What and Why:
We need to set defaults for apps deployed in Fly.io, see here!. This is also true for Laravel apps. So this PR contains changes to detect DBs and redis connection set in the local .env file, and set those as defaults for the Laravel Fly App, closely following the changes already available in the rails scanner.
How:
Checks the .env file of a Laravel directory and scans for db connection strings like "mysql", "pgsql", and "sqlite". If any of the connections above are available, sets it as the default DesiredDatabase value.
Additionally, detects the "redis" keyword and sets the configuration value of "RedisDesired" as true.
Related to:
Documentation
(will add docs in the laravel section of our docs)