-
Notifications
You must be signed in to change notification settings - Fork 40
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
chore: do not process webhook events for private repos #1162
Conversation
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
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.
I haven't tested the PR with a private repo, leaving comment just based on reading the PR.
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
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.
meh, I would have just did a separate if else branch for each class of error types, but this is good enough
@@ -79,6 +79,9 @@ type UpstreamRepositoryReference struct { | |||
// ErrRepoNotFound is returned when a repository is not found | |||
var ErrRepoNotFound = errors.New("repository not found") | |||
|
|||
// ErrRepoIsPrivate is returned when a repository is private | |||
var ErrRepoIsPrivate = errors.New("repository is private") |
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.
I'm not a fan of processing this as an error. In the near future we'll offer this as an enterprise feature and this route will sneak on us. Instead, let's handle it with an explicit if statement and no error for this.
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.
Make sense 👍 What do you mean by having no error - revert to returning a generic error or something else?
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.
FWIW, I don't treat returning an error in this case as..an error..just a way to signal a condition.
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.
Uhm.... as long as we don't forget later on...
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.
Okay, so I've added a separate if case so it's more explicit to not forget when we remove it later 👍
Thought of that, but since we don't do a lot of special things based on the error types I did not wanted to expand visually that section too much |
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
2970435
to
cdfa720
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.
I like this version
The following PR updates:
Related to #1083