Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add referenced workflows to WorkflowRun #2975

Merged
merged 1 commit into from
Oct 25, 2023
Merged

Add referenced workflows to WorkflowRun #2975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 40 additions & 33 deletions github/actions_workflow_runs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,39 +14,40 @@ import (

// WorkflowRun represents a repository action workflow run.
type WorkflowRun struct {
ID *int64 `json:"id,omitempty"`
Name *string `json:"name,omitempty"`
NodeID *string `json:"node_id,omitempty"`
HeadBranch *string `json:"head_branch,omitempty"`
HeadSHA *string `json:"head_sha,omitempty"`
RunNumber *int `json:"run_number,omitempty"`
RunAttempt *int `json:"run_attempt,omitempty"`
Event *string `json:"event,omitempty"`
DisplayTitle *string `json:"display_title,omitempty"`
Status *string `json:"status,omitempty"`
Conclusion *string `json:"conclusion,omitempty"`
WorkflowID *int64 `json:"workflow_id,omitempty"`
CheckSuiteID *int64 `json:"check_suite_id,omitempty"`
CheckSuiteNodeID *string `json:"check_suite_node_id,omitempty"`
URL *string `json:"url,omitempty"`
HTMLURL *string `json:"html_url,omitempty"`
PullRequests []*PullRequest `json:"pull_requests,omitempty"`
CreatedAt *Timestamp `json:"created_at,omitempty"`
UpdatedAt *Timestamp `json:"updated_at,omitempty"`
RunStartedAt *Timestamp `json:"run_started_at,omitempty"`
JobsURL *string `json:"jobs_url,omitempty"`
LogsURL *string `json:"logs_url,omitempty"`
CheckSuiteURL *string `json:"check_suite_url,omitempty"`
ArtifactsURL *string `json:"artifacts_url,omitempty"`
CancelURL *string `json:"cancel_url,omitempty"`
RerunURL *string `json:"rerun_url,omitempty"`
PreviousAttemptURL *string `json:"previous_attempt_url,omitempty"`
HeadCommit *HeadCommit `json:"head_commit,omitempty"`
WorkflowURL *string `json:"workflow_url,omitempty"`
Repository *Repository `json:"repository,omitempty"`
HeadRepository *Repository `json:"head_repository,omitempty"`
Actor *User `json:"actor,omitempty"`
TriggeringActor *User `json:"triggering_actor,omitempty"`
ID *int64 `json:"id,omitempty"`
Name *string `json:"name,omitempty"`
NodeID *string `json:"node_id,omitempty"`
HeadBranch *string `json:"head_branch,omitempty"`
HeadSHA *string `json:"head_sha,omitempty"`
RunNumber *int `json:"run_number,omitempty"`
RunAttempt *int `json:"run_attempt,omitempty"`
Event *string `json:"event,omitempty"`
DisplayTitle *string `json:"display_title,omitempty"`
Status *string `json:"status,omitempty"`
Conclusion *string `json:"conclusion,omitempty"`
WorkflowID *int64 `json:"workflow_id,omitempty"`
CheckSuiteID *int64 `json:"check_suite_id,omitempty"`
CheckSuiteNodeID *string `json:"check_suite_node_id,omitempty"`
URL *string `json:"url,omitempty"`
HTMLURL *string `json:"html_url,omitempty"`
PullRequests []*PullRequest `json:"pull_requests,omitempty"`
CreatedAt *Timestamp `json:"created_at,omitempty"`
UpdatedAt *Timestamp `json:"updated_at,omitempty"`
RunStartedAt *Timestamp `json:"run_started_at,omitempty"`
JobsURL *string `json:"jobs_url,omitempty"`
LogsURL *string `json:"logs_url,omitempty"`
CheckSuiteURL *string `json:"check_suite_url,omitempty"`
ArtifactsURL *string `json:"artifacts_url,omitempty"`
CancelURL *string `json:"cancel_url,omitempty"`
RerunURL *string `json:"rerun_url,omitempty"`
PreviousAttemptURL *string `json:"previous_attempt_url,omitempty"`
HeadCommit *HeadCommit `json:"head_commit,omitempty"`
WorkflowURL *string `json:"workflow_url,omitempty"`
Repository *Repository `json:"repository,omitempty"`
HeadRepository *Repository `json:"head_repository,omitempty"`
Actor *User `json:"actor,omitempty"`
TriggeringActor *User `json:"triggering_actor,omitempty"`
ReferencedWorkflows []*ReferencedWorkflow `json:"referenced_workflows,omitempty"`
}

// WorkflowRuns represents a slice of repository action workflow run.
Expand Down Expand Up @@ -104,6 +105,12 @@ type PendingDeploymentsRequest struct {
Comment string `json:"comment"`
}

type ReferencedWorkflow struct {
Path *string `json:"path,omitempty"`
SHA *string `json:"sha,omitempty"`
Comment on lines +109 to +110
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path and sha are required in the response schema, so they can be string instead of *string

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI - overall, we've been very lax on the "receive-side" of responses from the GitHub server over the years in terms of what the official docs say will definitely be returned versus what optionally might be returned.

One of the reasons we've done this is first, it has not always been clear from the documentation what will or will not be sent back, and we've also discovered experientially that there are sometimes surprises depending on where a particular struct is embedded (for example, if we were to add this to a different method's return struct).

Additionally, years back we added the helper methods to get field values that are pointers which help tremendously to avoid panics if something was expected but didn't show up in the response.

So I believe our official stance is to try and be super careful about the omitempty on the request side of the usage, but on the response side, we are happy to err on the side of more fields with pointers (rather than fewer).

So if @WillAbides had not mentioned this, I would have allowed this to go through, however I'm fine also with making the change since the official docs are hopefully correct and will not change soon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. That's good to read. I didn't realize the distinction between request and response fields.

I'm going to change my review to ✅ as soon as I hit submit on this comment. Then I might make a PR adding a note about this to CONTRIBUTING.md.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I wrote "official stance" above and that was probably too strongly worded 😄 .

The above is my "unofficial understanding" of how we are guiding the maintenance of this repo based upon years of observation.

The final authority is the original author of this repo, Will Norris, a friend and former coworker of mine, who asked me many years ago to help maintain this repo. I'm sure he listens in, as I hear from him every now and then, but I hate to ping him unless absolutely necessary. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmlewis thanks for the walkthrough in the past :) I'm happy to make any changes necessary. The reason I made them pointers is just to follow a patten with other required fields in WorkflowRun.

Ref *string `json:"ref,omitempty"`
}

func (s *ActionsService) listWorkflowRuns(ctx context.Context, endpoint string, opts *ListWorkflowRunsOptions) (*WorkflowRuns, *Response, error) {
u, err := addOptions(endpoint, opts)
if err != nil {
Expand Down
16 changes: 15 additions & 1 deletion github/actions_workflow_runs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,6 +761,13 @@ func TestWorkflowRun_Marshal(t *testing.T) {
SuspendedAt: &Timestamp{referenceTime},
URL: String("u2"),
},
ReferencedWorkflows: []*ReferencedWorkflow{
{
Path: String("rwfp"),
SHA: String("rwfsha"),
Ref: String("rwfref"),
},
},
}

want := `{
Expand Down Expand Up @@ -881,7 +888,14 @@ func TestWorkflowRun_Marshal(t *testing.T) {
"created_at": ` + referenceTimeStr + `,
"suspended_at": ` + referenceTimeStr + `,
"url": "u2"
}
},
"referenced_workflows": [
{
"path": "rwfp",
"sha": "rwfsha",
"ref": "rwfref"
}
]
}`

testJSONMarshal(t, u, want)
Expand Down
24 changes: 24 additions & 0 deletions github/github-accessors.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 30 additions & 0 deletions github/github-accessors_test.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading