Add custom domains CRUD #311
Conversation
|
|
||
| if err := ansi.Waiting(func() error { | ||
| var err error | ||
| list, err = cli.api.CustomDomain.List() |
There was a problem hiding this comment.
I noticed we're not passing the context downstream, to handle cancelation properly, eventually support timeouts from the root CLI command.
this can be done with the option
https://github.com/go-auth0/auth0/blob/5e36f59f4a866da1c4858b650302c04235dc3508/management/management.go#L399-L404
the command holds its context at cmd.Context()
It seems we're not doing this in any of our API invocations, so feel free to ignore this for now, I can tackle the update on a separated PR.
There was a problem hiding this comment.
Yes, that would need to be done in a separate PR.
| Use: "show", | ||
| Args: cobra.MaximumNArgs(1), | ||
| Short: "Show a custom domain", | ||
| Long: "Show a custom domain.", |
There was a problem hiding this comment.
I'm wondering if we really need long if its equal to short. do you know what's cobra doing differently with these values?
There was a problem hiding this comment.
Ideally we'd like that to be abstracted away and just use a builder that wraps cobra's with our own conventions. But that needs to be part of a larger refactor effort. For now the focus is on just hitting the feature goals
| list, err = cli.api.CustomDomain.List() | ||
| return err | ||
| }); err != nil { | ||
| return fmt.Errorf("An unexpected error occurred: %w", err) |
There was a problem hiding this comment.
shouldn't use
auth0-cli/internal/auth0/error.go
Line 5 in 77c96d3
There was a problem hiding this comment.
We're just using that method for panics for now.
There was a problem hiding this comment.
(so that Sentry has a stack trace when it recovers from the panic)
| return ansi.Spinner("Deleting custom domain", func() error { | ||
| _, err := cli.api.CustomDomain.Read(url.PathEscape(inputs.ID)) | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
is "not found" the only source of errors there?
There was a problem hiding this comment.
We only do that GET to find out if the resource exists. Granted, it can fail because other reasons as well (e.g. network error), but unfortunately, we have no way to tell them apart. The delete call does not fail if the resource didn't exist, it ends successfully. That might be the worse outcome in this case.
What do you suggest?
There was a problem hiding this comment.
Maybe rework the error message to mention that there are other potential reasons?
There was a problem hiding this comment.
yes, maybe just remove the The specified Id: %v doesn't exist and add the error msg?
| @@ -63,7 +63,7 @@ var ( | |||
| httpContentFormat = Flag{ | |||
| Name: "HTTP Content Format", | |||
| LongForm: "http-format", | |||
| Help: "HTTP Content-Format header. Possible values: JSONLINES, JSONARRAY, JSONOBJECT.", | |||
| Help: "HTTP Content-Format header. Possible values: jsonlines, jsonarray, jsonobject.", | |||
There was a problem hiding this comment.
is this related with the domains CRUD?
There was a problem hiding this comment.
No, but found out about that, and seemed too small a change to create a separate PR for, and having to find a reviewer. Ideally under different circumstances, I'd have made that into a separate PR. If you'd prefer it that way I can extract it.
| "gopkg.in/auth0.v5/management" | ||
| ) | ||
|
|
||
| type customDomainView struct { |
There was a problem hiding this comment.
what do you think about and approach like this to simplify here and there?
type customDomainView struct {
raw *management.CustomDomain
}
func (v *customDomainView) AsTableRow() []string {
return []string{
ansi.Faint(v.raw.ID),
v.raw.Domain,
v.raw.Status,
}
}There was a problem hiding this comment.
I think keeping a separate view model that does not know about the underlying DTO is something that will make it easier and simpler to decouple the SDK in the future (if the project gets to that stage). Otherwise the data layer gets a (bigger) hard coupling with the view layer.
There was a problem hiding this comment.
I mean, both layers are already coupled, but doing that will make it worse.
Description
This PR adds the following commands:
auth0 branding domains listauth0 branding domains showauth0 branding domains createauth0 branding domains deleteauth0 branding domains verifyThe
updatecommand cannot be added ATM because the Go SDK does not support that operation.Testing
Checklist
master