-
Notifications
You must be signed in to change notification settings - Fork 24
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
fix(cmd): addons-info working for non-db addons #1055
Conversation
0ac01c3
to
2bffb48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions and nitpicks otherwise LGTM 👍
db/utils.go
Outdated
package db | ||
|
||
var ( | ||
SupportedDatabases = []string{"PostgreSQL", "InfluxDB", "MongoDB", "MySQL"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: no Redis? no Elasticsearch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that a regression, my bad. Nice catch
return strings.EqualFold(s, addonInfo.AddonProvider.ID) | ||
}) | ||
|
||
dbInfo := [][]string{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: dbInfo
is used for database information but also for other addon information after the condition.
I suggest:
dbInfo := [][]string{} | |
addonInfo := [][]string{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable only contains database information, or I don't see which condition are you talking about. I surely miss something here, could you point me where exactly please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 my bad, I misread the code. You're right!
088c846
to
2dc2c1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
return strings.EqualFold(s, addonInfo.AddonProvider.ID) | ||
}) | ||
|
||
dbInfo := [][]string{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 my bad, I misread the code. You're right!
Fixes #1054