store: refactor GetRemote #2975
Conversation
f0b7948 to
b061cd0
Compare
| "time" | ||
| ) | ||
|
|
||
| var ( | ||
| ErrRemoteNotFound = errors.New("remote with the given ACI URL not found") |
There was a problem hiding this comment.
Given that we are already planning to integrate non-ACI stores, can we remove ACI references from there? A generic remote target not found should suffice.
There was a problem hiding this comment.
Just a personal taste, but I find easier to just return nil if not found and check it in the caller instead of the error type checking. Many pkgs (with the exception of os that wraps a syscall error for ErrNotFound) do this.
There was a problem hiding this comment.
I don't agree with returning nil to check whether a function succeeded or failed. We already have an error field in the function signature
There was a problem hiding this comment.
I changed my mind 😄 . And to keep a common pattern, when possible, in #3071 I started using the same ErrNotFound approach.
|
This is in line with what we discussed and looks fine, just one minor comment inline. Let's give a chance to @sgotti to have a look before merging it, as I think he's already touching this code. |
| @@ -48,23 +53,26 @@ func remoteRowScan(rows *sql.Rows, remote *Remote) error { | |||
|
|
|||
| // GetRemote tries to retrieve a remote with the given aciURL. found will be | |||
| // false if remote doesn't exist. | |||
| func GetRemote(tx *sql.Tx, aciURL string) (remote *Remote, found bool, err error) { | |||
| remote = &Remote{} | |||
| func GetRemote(tx *sql.Tx, aciURL string) (*Remote, error) { | |||
There was a problem hiding this comment.
Godoc needs to be fixed with the new NotFound type (nil or error, see above comment)
|
comments are addressed, PTAL |
|
Low priority, moved to next milestone while waiting for last review. |
|
@sgotti are you fine with this? |
|
After squashing the two commits it LGTM. |
|
@sgotti commits are independent now, PTAL (2nd commit is a minor cleanup commit of the codebase) |
|
@tmrts Thanks! LGTM. |
| @@ -551,15 +551,21 @@ func (s *Store) RemoveACI(key string) error { | |||
|
|
|||
| // GetRemote tries to retrieve a remote with the given ACIURL. found will be | |||
|
@tmrts up for fixing the nit? |
Removes the 2nd argument signalling whether the remote is found or not and returns a distinct error instead.
|
@s-urbaniak sorry I've missed @jonboulle 's comment, PTAL |
|
lgtm, thanks! |
Removes the 2nd argument signalling whether the remote is found or not
and returns a distinct error instead.
cc @lucab @sgotti