fix(cli attestation): handle non-merged branches #451
Conversation
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
| return nil, fmt.Errorf("getting remotes: %w", err) | ||
| // go-git does an additional validation that the branch is pushed upstream | ||
| // we do not care about that use-case, so we ignore the error | ||
| return c, nil |
There was a problem hiding this comment.
I know this is swallowing other errors, but I decided to go this way since
- That error type is not exposed in the upstream library
- We really shouldn't block on remotes lookups for any reason.
There was a problem hiding this comment.
I understand the point, but I can see that the function repo.Remotes() can return more than one type of error. I think you want to swallow only if err is ErrRemoteNotFound or equivalent, am I right?
There was a problem hiding this comment.
That's right, that's what I tried to explain in my comment. "I know this is swallowing other errors"
I just thought it was not worth inspecting them as long as we log them (see the logger.Warn), and I do not see a reason why we should fail in any circumstance.
In any case, let me revisit it.
Thanks!
There was a problem hiding this comment.
Ok so you are ubersure that no matter what happens, we can keep going.
Gotcha, I'll approve now that I'm sure <3
There was a problem hiding this comment.
ptal at the change now.
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
Shows an error message instead of blocking the attestation