feat!: Refactor actions variables to pass request by value#4346
Conversation
Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>
gmlewis
changed the title
gmlewis
added
NeedsReview
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4346 +/- ##
=======================================
Coverage 97.50% 97.51%
=======================================
Files 193 193
Lines 19481 19519 +38
=======================================
+ Hits 18995 19033 +38
Misses 269 269
Partials 217 217 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @stevehipwell!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
| func (s *ActionsService) UpdateOrgVariable(ctx context.Context, org string, variable *ActionsVariable) (*Response, error) { | ||
| if variable == nil { | ||
| return nil, errors.New("variable must be provided") | ||
| func (s *ActionsService) UpdateOrgVariable(ctx context.Context, org, name string, body OrgActionsVariableRequest) (*Response, error) { |
There was a problem hiding this comment.
We can't reuse OrgActionsVariableRequest because all fields are optional, according to the schema:
We need a separate struct.
There was a problem hiding this comment.
Good spot, I'll change this.
| url := fmt.Sprintf("repos/%v/%v/environments/%v/variables/%v", owner, repo, env, variable.Name) | ||
| return s.patchVariable(ctx, url, variable) | ||
| } | ||
| func (s *ActionsService) UpdateEnvVariable(ctx context.Context, owner, repo, env, name string, body ActionsVariableRequest) (*Response, error) { |
There was a problem hiding this comment.
We can't share the same ActionsVariableRequest for create and update. Should be a struct with optional fields.
| func (s *ActionsService) CreateOrgVariable(ctx context.Context, org string, body *ActionsVariable) (*Response, error) { | ||
| url := fmt.Sprintf("orgs/%v/actions/variables", org) | ||
| return s.postVariable(ctx, url, body) | ||
| func (s *ActionsService) CreateOrgVariable(ctx context.Context, org string, body OrgActionsVariableCreateRequest) (*Response, error) { |
There was a problem hiding this comment.
Just a minor tweak for consistency:
| func (s *ActionsService) CreateOrgVariable(ctx context.Context, org string, body OrgActionsVariableCreateRequest) (*Response, error) { | |
| func (s *ActionsService) CreateOrgVariable(ctx context.Context, org string, body ActionsCreateOrgVariableRequest) (*Response, error) { |
The same for the rest body parameters.
|
@alexandear do you want me to update the type names for the secrets & variables in a new PR? I was expecting to do it on this PR, but it's already been merged. |
|
Whups. Sorry. |
@stevehipwell yes, let's update. |
|
@alexandear the changes are in #4348. |
3 participants
BREAKING CHANGE:
ActionsServicemethods involving variables have new params and return values.This PR updates the actions variable create & update functions to use a value parameter (see #3644). This aligns actions variables with the changes made to actions secrets in #4335.