-
Notifications
You must be signed in to change notification settings - Fork 41
Fix incorrect workflow_id type for workflow_run table #558
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ package github | |
|
|
||
| import ( | ||
| "context" | ||
| "strconv" | ||
| "time" | ||
|
|
||
| "github.com/google/go-github/v55/github" | ||
|
|
||
|
|
@@ -24,8 +24,11 @@ func tableGitHubActionsRepositoryWorkflowRun() *plugin.Table { | |
| {Name: "workflow_id", Require: plugin.Optional}, | ||
| {Name: "event", Require: plugin.Optional}, | ||
| {Name: "head_branch", Require: plugin.Optional}, | ||
| {Name: "head_sha", Require: plugin.Optional}, | ||
| {Name: "status", Require: plugin.Optional}, | ||
| {Name: "conclusion", Require: plugin.Optional}, | ||
| {Name: "actor_login", Require: plugin.Optional}, | ||
| {Name: "created_at", Require: plugin.Optional, Operators: []string{">", ">=", "<", "<=", "="}}, | ||
| }, | ||
| }, | ||
| Get: &plugin.GetConfig{ | ||
|
|
@@ -40,9 +43,9 @@ func tableGitHubActionsRepositoryWorkflowRun() *plugin.Table { | |
| Columns: commonColumns([]*plugin.Column{ | ||
| // Top columns | ||
| {Name: "repository_full_name", Type: proto.ColumnType_STRING, Transform: transform.FromQual("repository_full_name"), Description: "Full name of the repository that specifies the workflow run."}, | ||
| {Name: "id", Type: proto.ColumnType_INT, Description: "The unque identifier of the workflow run."}, | ||
| {Name: "id", Type: proto.ColumnType_INT, Description: "The unique identifier of the workflow run."}, | ||
| {Name: "event", Type: proto.ColumnType_STRING, Description: "The event for which workflow triggered off."}, | ||
| {Name: "workflow_id", Type: proto.ColumnType_STRING, Description: "The workflow id of the workflow run."}, | ||
| {Name: "workflow_id", Type: proto.ColumnType_INT, Description: "The workflow id of the workflow run."}, | ||
| {Name: "node_id", Type: proto.ColumnType_STRING, Description: "The node id of the workflow run."}, | ||
| {Name: "conclusion", Type: proto.ColumnType_STRING, Description: "The conclusion for workflow run."}, | ||
| {Name: "status", Type: proto.ColumnType_STRING, Description: "The status of the workflow run."}, | ||
|
|
@@ -67,7 +70,7 @@ func tableGitHubActionsRepositoryWorkflowRun() *plugin.Table { | |
| {Name: "pull_requests", Type: proto.ColumnType_JSON, Description: "The pull request details for the workflow run."}, | ||
| {Name: "repository", Type: proto.ColumnType_JSON, Description: "The repository info for the workflow run."}, | ||
| {Name: "run_attempt", Type: proto.ColumnType_INT, Description: "The attempt number of the workflow run."}, | ||
| {Name: "run_started_at", Type: proto.ColumnType_TIMESTAMP, Transform: transform.FromField("RunStartedAt").Transform(convertTimestamp), Description: "Time when the workflow run was started."}, | ||
| {Name: "run_started_at", Type: proto.ColumnType_TIMESTAMP, Transform: transform.FromField("RunStartedAt").NullIfZero().Transform(convertTimestamp), Description: "Time when the workflow run was started."}, | ||
| {Name: "updated_at", Type: proto.ColumnType_TIMESTAMP, Transform: transform.FromField("UpdatedAt").Transform(convertTimestamp), Description: "Time when the workflow run was updated."}, | ||
| {Name: "actor", Type: proto.ColumnType_JSON, Description: "The user whom initiated the first instance of this workflow run."}, | ||
| {Name: "actor_login", Type: proto.ColumnType_STRING, Description: "The login of the user whom initiated the first instance of the workflow run.", Transform: transform.FromField("Actor.Login")}, | ||
|
|
@@ -85,29 +88,60 @@ func tableGitHubRepoWorkflowRunList(ctx context.Context, d *plugin.QueryData, h | |
| opts := &github.ListWorkflowRunsOptions{ | ||
| ListOptions: github.ListOptions{PerPage: 100}, | ||
| } | ||
| equalQuals := d.EqualsQuals | ||
| if equalQuals["event"] != nil { | ||
| if equalQuals["event"].GetStringValue() != "" { | ||
| opts.Event = equalQuals["event"].GetStringValue() | ||
| } | ||
| if event := d.EqualsQualString("event"); event != "" { | ||
| opts.Event = event | ||
| } | ||
| if equalQuals["head_branch"] != nil { | ||
| if equalQuals["head_branch"].GetStringValue() != "" { | ||
| opts.Branch = equalQuals["head_branch"].GetStringValue() | ||
| } | ||
| if branch := d.EqualsQualString("head_branch"); branch != "" { | ||
| opts.Branch = branch | ||
| } | ||
| if equalQuals["status"] != nil { | ||
| if equalQuals["status"].GetStringValue() != "" { | ||
| opts.Status = equalQuals["status"].GetStringValue() | ||
| } | ||
| if headSha := d.EqualsQualString("head_sha"); headSha != "" { | ||
| opts.HeadSHA = headSha | ||
| } | ||
| if status := d.EqualsQualString("status"); status != "" { | ||
| opts.Status = status | ||
| } | ||
| if actorLogin := d.EqualsQualString("actor_login"); actorLogin != "" { | ||
| opts.Actor = actorLogin | ||
| } | ||
|
|
||
| // Status param can take the value from both status and conclusion column | ||
| // https://docs.github.com/en/rest/reference/actions#workflow-runs | ||
| if equalQuals["conclusion"] != nil { | ||
| // https://docs.github.com/en/rest/actions/workflow-runs#list-workflow-runs-for-a-repository | ||
| if conclusion := d.EqualsQualString("conclusion"); conclusion != "" { | ||
| if opts.Status == "" { | ||
| if equalQuals["conclusion"].GetStringValue() != "" { | ||
| opts.Status = equalQuals["conclusion"].GetStringValue() | ||
| opts.Status = conclusion | ||
| } | ||
| } | ||
|
|
||
| // Convert quals into GitHub search syntax | ||
| // https://docs.github.com/en/search-github/getting-started-with-searching-on-github/understanding-the-search-syntax#query-for-dates | ||
| if createdAt := d.Quals["created_at"]; createdAt != nil { | ||
| var lowerBound, upperBound time.Time | ||
| for _, q := range createdAt.Quals { | ||
| t := q.Value.GetTimestampValue().AsTime() | ||
| // Note: This logic returns boundary rows regardless of operator, but qual recheck will filter those out client-side | ||
| // Keep the _latest_ lower bound and _earliest_ upper bound in case of overlapping filters | ||
| switch q.Operator { | ||
| case "=": | ||
| opts.Created = t.Format(time.DateOnly) | ||
| case ">", ">=": | ||
| if lowerBound.IsZero() || t.After(lowerBound) { | ||
| lowerBound = t | ||
| } | ||
| case "<", "<=": | ||
| if upperBound.IsZero() || t.Before(upperBound) { | ||
| upperBound = t | ||
| } | ||
| } | ||
| } | ||
| if opts.Created == "" { | ||
| var lower, upper = lowerBound.Format(time.RFC3339), upperBound.Format(time.RFC3339) | ||
| switch { | ||
| case !lowerBound.IsZero() && !upperBound.IsZero(): | ||
| opts.Created = lower + ".." + upper | ||
| case !lowerBound.IsZero(): | ||
| opts.Created = ">=" + lower | ||
| case !upperBound.IsZero(): | ||
| opts.Created = "<=" + upper | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+117
to
147
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This range building is now order dependent. The quals arrive in query order, so Since the filter is pushed down to the API, postgres never gets the rows back to recheck, so they're silently dropped. Three or more Can you collect the bounds first, then build the string once? Something like: if createdAt := d.Quals["created_at"]; createdAt != nil {
var lower, upper string
for _, q := range createdAt.Quals {
t := q.Value.GetTimestampValue().AsTime()
switch q.Operator {
case "=":
opts.Created = t.Format(time.DateOnly)
case ">", ">=":
lower = t.Format(time.RFC3339)
case "<", "<=":
upper = t.Format(time.RFC3339)
}
}
if opts.Created == "" {
switch {
case lower != "" && upper != "":
opts.Created = lower + ".." + upper
case lower != "":
opts.Created = ">=" + lower
case upper != "":
opts.Created = "<=" + upper
}
}
}One thing worth a code comment either way:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think the order of the quals actually makes a difference and changing it does work in the bare API and in this PR: Shell example with $ gh api repos/turbot/steampipe/actions/runs -X GET -f created=2026-02-01T00:00:00Z..2026-01-01T00:00:00Z -F per_page=1
{
"total_count": 44,
"workflow_runs": [
{
"id": 21541410334,
"name": "30 - Admin: Stale Issues and PRs",
....
}
$ gh api repos/turbot/steampipe/actions/runs -X GET -f created=2026-01-01T00:00:00Z..2026-02-01T00:00:00Z -F per_page=1
{
"total_count": 44,
"workflow_runs": [
{
"id": 21541410334,
"name": "30 - Admin: Stale Issues and PRs",
...
}
SQL example with these changes: select
*
from
github_actions_repository_workflow_run
where
repository_full_name = 'turbot/steampipe'
and created_at <= '2026-02-01T00:00:00Z'
and created_at >= '2026-01-01T00:00:00Z'
limit 1;
+----------------------+----------------------+----------------+----------+-------------+----------------------------+------------+-----------+------------+--->
| login_id | repository_full_name | id | event | workflow_id | node_id | conclusion | status | run_number | ar>
+----------------------+----------------------+----------------+----------+-------------+----------------------------+------------+-----------+------------+--->
| MDQ6VXNlcjM2MjM0MjY= | turbot/steampipe | 21,541,410,334 | schedule | 172,982,979 | WFR_kwLOE7GVQM8AAAAFA_fWHg | success | completed | 209 | ht>
| | | | | | | | | | >
| | | | | | | | | | >
| | | | | | | | | | >
+----------------------+----------------------+----------------+----------+-------------+----------------------------+------------+-----------+------------+--->
(END)
Maybe a dumb question, but would we ever use three or more clauses on To be clear, I'm not arguing against this suggestion and I'm happy to incorporate it, but I'm more asking about whether this will work as we expect and push down the
This is a good point. I hadn't considered the boundary row condition, but I think the approach of adding/subtracting a second (similar to here [2]) might work, if you agree? [2] https://github.com/turbot/steampipe-plugin-github/blob/main/github/table_github_commit.go#L77
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right on the reversed range, thanks for testing that. I reproduced it, both orders return the same 44 results, so GitHub normalizes it (undocumented, but it works). Looking at the 3 quals issue, I had done some testing and found it also fires with just two. The current code assumes the second qual is always the opposite bound of the first, so two same-direction quals break it. For instance, this where created_at >= '2026-01-01' and created_at > '2026-01-15'Builds The query silently returns zero rows. Same story with two upper bounds, or an = qual followed by a range qual ( This shape shows up without anyone typing it, as a view or CTE with a baked-in window ( (Using 3+ quals also has the same issue today) For the pushdown question, there's no need for multiple API calls. We can collapse all the quals to a single lower and upper bound, build one range string, and then make one call. The API filter just needs to be a superset of what the quals allow, and the qual recheck drops anything extra. On the boundary rows, I'd skip the +/- 1 second adjustment from the commit table. It has an edge case for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
First of all, thank you so much for the feedback! Extremely helpful since I'm not that familiar with Postgres. Second, this makes *complete sense. I'll make these changes, but probably add a bit of logic to make sure we account for (possibly) redundant filters and reduce rows client-side.
Got it. Thanks for clarifying! I think I did not explain what I meant correctly:
You got it. |
||
|
|
@@ -119,16 +153,7 @@ func tableGitHubRepoWorkflowRunList(ctx context.Context, d *plugin.QueryData, h | |
| } | ||
| } | ||
|
|
||
| var workflowId int64 | ||
| if equalQuals["workflow_id"] != nil { | ||
| if equalQuals["workflow_id"].GetStringValue() != "" { | ||
| workflowId_, err := strconv.ParseInt(equalQuals["workflow_id"].GetStringValue(), 10, 64) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| workflowId = workflowId_ | ||
| } | ||
| } | ||
| workflowId := d.EqualsQuals["workflow_id"].GetInt64Value() | ||
|
|
||
| for { | ||
| var ( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.