feat: PR labels (#3002) #3118
feat: PR labels (#3002) #3118
Conversation
✅ Deploy Preview for docs-kargo-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3118 +/- ##
==========================================
+ Coverage 51.16% 51.34% +0.17%
==========================================
Files 286 286
Lines 25912 25940 +28
==========================================
+ Hits 13259 13319 +60
+ Misses 11950 11908 -42
- Partials 703 713 +10 ☔ View full report in Codecov by Sentry. |
|
@krancour I went down the rabbit hole of adding tests for the GitHub provider. Please let me know if this is generally going in a direction you like or whether the added complexity for these new tests is too much. |
I like how you're approaching it, but the thing I worry about is that for something as complex as the GitHub APIs, it's plausible for the mock to accidentally do something that's inconsistent with the real API's behavior with the result being tests that pass but shouldn't. We eventually need to figure out how to have dedicated repos for each provider perpetually available to test against or have a way to provision repos just-in-time and clean them up afterwards. For now, I wouldn't let the above get in the way. |
By that do you mean you wouldn't add tests at all or just leave them as proposed in the PR and then add some real integration tests later? |
| type githubClientWrapper struct { | ||
| client *github.Client | ||
| } | ||
|
|
||
| func (g githubClientWrapper) CreatePullRequest(ctx context.Context, owner string, repo string, pull *github.NewPullRequest) (*github.PullRequest, *github.Response, error) { | ||
| return g.client.PullRequests.Create(ctx, owner, repo, pull) | ||
| } | ||
|
|
||
| func (g githubClientWrapper) ListPullRequests(ctx context.Context, owner string, repo string, opts *github.PullRequestListOptions) ([]*github.PullRequest, *github.Response, error) { | ||
| return g.client.PullRequests.List(ctx, owner, repo, opts) | ||
| } | ||
|
|
||
| func (g githubClientWrapper) GetPullRequests(ctx context.Context, owner string, repo string, number int) (*github.PullRequest, *github.Response, error) { | ||
| return g.client.PullRequests.Get(ctx, owner, repo, number) | ||
| } | ||
|
|
||
| func (g githubClientWrapper) AddLabelsToIssue(ctx context.Context, owner string, repo string, number int, labels []string) ([]*github.Label, *github.Response, error) { | ||
| return g.client.Issues.AddLabelsToIssue(ctx, owner, repo, number, labels) | ||
| } | ||
|
|
There was a problem hiding this comment.
Instead of creating a client wrapper, could the githubClient interface just be the relevant subset of the methods the client already actually has? This way we can just use the existing (unwrapped) client as the implementation of the githubClient, which is the interface we'll code to?
There was a problem hiding this comment.
Oh... it's because of this p.client.PullRequests.Create 🤦♂️
That makes my suggestion rather difficult to pull off.
Nevermind.
There was a problem hiding this comment.
Believe me, I tried 😄
Dealer's choice. I'm fine either way. btw... #3128 just got merged, so you probably have to tweak that new provider a bit as well. Hopefully that's no too involved. |
Signed-off-by: Dominik Münch <d.muench@celonis.com>
Signed-off-by: Dominik Münch <d.muench@celonis.com>
Signed-off-by: Dominik Münch <d.muench@celonis.com>
|
@diegocaspi thank you for some excellent work on this! |
Signed-off-by: Dominik Münch <d.muench@celonis.com>
Signed-off-by: Dominik Münch <d.muench@celonis.com>
closes #3002