Uh oh!
There was an error while loading. Please reload this page.
Fix CreateOrUpdateOrgSecret regression introduced in v53 - #2817
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
| putSecret := func(ctx context.Context, url string, eSecret interface{}) (*Response, error) { | ||
| req, err := s.client.NewRequest("PUT", url, eSecret) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return s.client.Do(ctx, req, nil) | ||
| } |
There was a problem hiding this comment.
Created this inline for now as it's the only place it's used. Happy to pull it out as an unexported function to match the putSecret package function (maybe putOrgSecret). That would require though defining a package type for params (for used as an input argument). I thought it was an overkill for now, so left it as such.
There was a problem hiding this comment.
There's no need for the inline function here. Let's just please delete lines 160 and 167, and move lines 161-166 down to replace line 170 below.
There was a problem hiding this comment.
Sounds good - was trying to keep the symmetry with what was previously there. Removed!
ac1e88d to
bca8d40CompareCodecov Report
@@ Coverage Diff @@## master #2817 +/- ##
=======================================
Coverage 98.05% 98.06% =======================================
Files 136 136 Lines 12263 12279 +16 =======================================
+ Hits 12025 12041 +16
Misses 162 162 Partials 76 76
|
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @frankywahl!
Just a few tweaks, please, then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.
| putSecret := func(ctx context.Context, url string, eSecret interface{}) (*Response, error) { | ||
| req, err := s.client.NewRequest("PUT", url, eSecret) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return s.client.Do(ctx, req, nil) | ||
| } |
There was a problem hiding this comment.
There's no need for the inline function here. Let's just please delete lines 160 and 167, and move lines 161-166 down to replace line 170 below.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
`CreateOrUpdateOrgSecret`'s API uses `string` for selected repository IDS while other APIs use `int`. This fixes the issue locally by implementing its own type
gmlewis
commented
Jun 23, 2023
Also, in the future, please don't use force-push in PRs to this repo, as it makes reviewing more challenging to see what has changed since the last review. |
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @frankywahl !
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
gmlewis
commented
Jun 27, 2023
Thank you, @valbeat ! |
CreateOrUpdateOrgSecret's API usesstringfor selected repository IDS while other APIs useint. This fixes the issue locally by implementing its own type.fixes#2816