feat: addon param can be an addon type name #983
Conversation
curzolapierre
force-pushed
the
feat/get_addon_by_type
branch
3 times, most recently
from
99cbbc5 to
654fe93
Compare
curzolapierre
force-pushed
the
feat/get_addon_by_type
branch
from
654fe93 to
a0bfaea
Compare
curzolapierre
changed the title
utils/addon_uuid.go
Outdated
|
|
||
| func GetAddonUUIDFromType(ctx context.Context, app, addonUUIDorType string) (string, error) { | ||
| // If addon does not contain a UUID, we consider it contains an addon type (e.g. MongoDB) | ||
| if !strings.HasPrefix(addonUUIDorType, "ad-") { |
There was a problem hiding this comment.
issue(blocking): I thinks it should be
if strings.HasPrefix(addonUUIDorType, "ad-") {Because, if it does have the prefix ad-, it means it's the addon UUID and we don't need to retrieve the UUID from the type.
There was a problem hiding this comment.
Oh nice catch, last minute copy/paste typo here
utils/addon_uuid.go
Outdated
|
|
||
| addonsClient, err := config.ScalingoClient(ctx) | ||
| if err != nil { | ||
| return "", errors.Notef(ctx, err, "unable to get Scalingo client") |
There was a problem hiding this comment.
issue(non-blocking): errors should be wrap with errors.Wrapf
(c.f. https://www.notion.so/scalingooriginal/Go-Error-Management-1ca42114c730415e95755027935eaccc?pvs=4#e9785e8022e5415eb211e966deefcea2)
utils/addon_uuid.go
Outdated
|
|
||
| "es": "elasticsearch", | ||
| } | ||
| addonTypeAlias, isAlias := aliases[addonType] |
There was a problem hiding this comment.
nitpick: It's not the alias you get here. You retrieve the addonType from an alias
There was a problem hiding this comment.
I'm not sure about what you are meaning here. Would you have a suggestion?
There was a problem hiding this comment.
Renaming addonTypeAlias variable to addonTypeFromAlias should be ok IMHO 🙂
utils/addon_uuid.go
Outdated
| "github.com/Scalingo/go-utils/errors/v2" | ||
| ) | ||
|
|
||
| func GetAddonUUIDFromType(ctx context.Context, app, addonUUIDorType string) (string, error) { |
There was a problem hiding this comment.
nitpick: I found hard to read the variable addonUUIDorType maybe rename it to addonTypeOrUUID.
If you have a better name, use it 🙂
curzolapierre
force-pushed
the
feat/get_addon_by_type
branch
from
a0bfaea to
a391716
Compare
curzolapierre
force-pushed
the
feat/get_addon_by_type
branch
from
a391716 to
2ff4d7f
Compare
utils/addon_uuid.go
Outdated
|
|
||
| "es": "elasticsearch", | ||
| } | ||
| addonTypeAlias, isAlias := aliases[addonType] |
There was a problem hiding this comment.
Renaming addonTypeAlias variable to addonTypeFromAlias should be ok IMHO 🙂
|
|
||
| func GetAddonUUIDFromType(ctx context.Context, app, addonTypeOrUUID string) (string, error) { | ||
| // If addon does not contain a UUID, we consider it contains an addon type (e.g. MongoDB) | ||
| if strings.HasPrefix(addonTypeOrUUID, "ad-") { |
There was a problem hiding this comment.
question: Is there a way to verify correctly if it's an UUID?
Because if I pass in argument ad-toto, we will do a request to retrieve list of addons for nothing 🙁
There was a problem hiding this comment.
The test is already done in db-api side. It would returns an HTTP 404
curzolapierre
force-pushed
the
feat/get_addon_by_type
branch
from
2ff4d7f to
9fe2d45
Compare
|
Nitpick: your branch name is incorrect |
cmd/flags.go
Outdated
| var err error | ||
| addonUUID, err = utils.GetAddonUUIDFromType(c.Context, app, addonName) | ||
| if err != nil { | ||
| fmt.Println("Unable to get the addon UUID based on its type:", err) |
There was a problem hiding this comment.
From what I remember error message in the CLI must be displayed calling io.Error (https://github.com/Scalingo/cli/blob/7cf4f23d162908d11803e5c03d15f977d53059f7/io/status.go).
curzolapierre
force-pushed
the
feat/get_addon_by_type
branch
from
9fe2d45 to
11cf3e7
Compare
Fixes #937