Wire in GitHub App authentication for the commenter and update job flags #32806
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,14 +29,12 @@ import ( | |
| "fmt" | ||
| "log" | ||
| "math/rand" | ||
| "regexp" | ||
| "strconv" | ||
| "strings" | ||
| "text/template" | ||
| "time" | ||
|
|
||
| "sigs.k8s.io/prow/pkg/flagutil" | ||
| "sigs.k8s.io/prow/pkg/github" | ||
| ) | ||
|
|
@@ -58,9 +56,9 @@ const ( | |
| ) | ||
|
|
||
| func flagOptions() options { | ||
| o := options{} | ||
|
|
||
| flag.StringVar(&o.org, "org", "", "GitHub organization (required when using GitHub App credentials)") | ||
| flag.StringVar(&o.query, "query", "", "See https://help.github.com/articles/searching-issues-and-pull-requests/") | ||
| flag.DurationVar(&o.updated, "updated", 2*time.Hour, "Filter to issues unmodified for at least this long if set") | ||
| flag.BoolVar(&o.includeArchived, "include-archived", false, "Match archived issues if set") | ||
|
|
@@ -70,10 +68,10 @@ func flagOptions() options { | |
| flag.StringVar(&o.comment, "comment", "", "Append the following comment to matching issues") | ||
| flag.BoolVar(&o.useTemplate, "template", false, templateHelp) | ||
| flag.IntVar(&o.ceiling, "ceiling", 3, "Maximum number of issues to modify, 0 for infinite") | ||
|
There was a problem hiding this comment. Could we keep these temporarily and backfill I'd prefer to do that and emit a warning about eventually (a month? two?) removing these options, to give folks at least a chance to notice and fixup their configs. With a reminder comment someone will eventually notice and remove the code after the date. There was a problem hiding this comment. (as written this will also break kubernetes, unless quickly followed with a successful image upgrade ...) There was a problem hiding this comment. I've started by wiring the old flags, but some of the fields were private (in the new prow utils config). Let me give it a second try with some extra variables and syncing it back. |
||
| flag.BoolVar(&o.random, "random", false, "Choose random issues to comment on from the query") | ||
|
|
||
| o.github.AddFlags(flag.CommandLine) | ||
|
|
||
| flag.Parse() | ||
| return o | ||
| } | ||
|
|
@@ -88,17 +86,16 @@ type meta struct { | |
| type options struct { | ||
| ceiling int | ||
| comment string | ||
| org string | ||
| includeArchived bool | ||
| includeClosed bool | ||
| includeLocked bool | ||
| useTemplate bool | ||
| query string | ||
| updated time.Duration | ||
| confirm bool | ||
| random bool | ||
| github flagutil.GitHubOptions | ||
| } | ||
|
|
||
| func parseHTMLURL(url string) (string, string, int, error) { | ||
|
|
@@ -151,8 +148,8 @@ func makeQuery(query string, includeArchived, includeClosed, includeLocked bool, | |
| } | ||
|
|
||
| type client interface { | ||
| CreateComment(org, repo string, number int, comment string) error | ||
| FindIssuesWithOrg(org, query, sort string, asc bool) ([]github.Issue, error) | ||
| } | ||
|
|
||
| func main() { | ||
|
|
@@ -162,31 +159,22 @@ func main() { | |
| if o.query == "" { | ||
| log.Fatal("empty --query") | ||
| } | ||
| if o.github.TokenPath == "" && o.github.AppID == "" { | ||
| log.Fatal("no github authentication options specified") | ||
| } | ||
| if o.github.AppID != "" && o.org == "" { | ||
| log.Fatal("using github appid requires using --org flag") | ||
| } | ||
| if o.comment == "" { | ||
| log.Fatal("empty --comment") | ||
| } | ||
|
|
||
| githubOptsErr := o.github.Validate(true) | ||
| if githubOptsErr != nil { | ||
| log.Fatalf("Error validating github options: %v", githubOptsErr) | ||
| } | ||
|
|
||
| c, err := o.github.GitHubClient(!o.confirm) | ||
| if err != nil { | ||
| log.Fatalf("Failed to construct GitHub client: %v", err) | ||
| } | ||
|
|
@@ -202,7 +190,7 @@ func main() { | |
| asc = true | ||
| } | ||
| commenter := makeCommenter(o.comment, o.useTemplate) | ||
| if err := run(c, o.org, query, sort, asc, o.random, commenter, o.ceiling); err != nil { | ||
| log.Fatalf("Failed run: %v", err) | ||
| } | ||
| } | ||
|
|
@@ -221,9 +209,9 @@ func makeCommenter(comment string, useTemplate bool) func(meta) (string, error) | |
| } | ||
| } | ||
|
|
||
| func run(c client, org, query, sort string, asc, random bool, commenter func(meta) (string, error), ceiling int) error { | ||
| log.Printf("Searching: %s", query) | ||
| issues, err := c.FindIssuesWithOrg(org, query, sort, asc) | ||
|
There was a problem hiding this comment. Interesting, I don't remember this. The need for the org arg could be avoided by extracting it from the query, it should always have one of There was a problem hiding this comment. the JWT tokes signed for the GH app are signed only for the installation ID which is per org, the query doesn't really matter afaik, so you can only query a single org in a single job with GH apps There was a problem hiding this comment. The point is right now you have to specify the same org in both the query and the CLI arg, it would be a better UX if we extracted the org from the query so it doesn't have to be duplicated into the CLI arg. There was a problem hiding this comment. It has crossed my mind but: There was a problem hiding this comment. I don't think its very complex, you just have to extract all There was a problem hiding this comment. I don't think this should be coupled to the GitHub search query syntax that can change over time. We'd always have to keep up. There is also set logic that you have to account for, like is:issue team:kubernetes/sig-testing -org:kubernetes-sigs |
||
| if err != nil { | ||
| return fmt.Errorf("search failed: %w", err) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't a sufficiently safe way to make this change, as we're not running from source here, see the
imageabove