Wire in GitHub App authentication for the commenter and update job flags #32806
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
requested review from
cjwagner,
dims,
MadhavJivrajani and
Priyankasaggu11929
k8s-ci-robot
added
area/robots
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tnozicka The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tnozicka
force-pushed
the
commenter-app-auth
branch
from
f36878a
to
2099c79
Compare
|
Here is a successful run with an image built from this PR using the GitHub App credentials |
| @@ -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") | |||
| flag.Var(&o.endpoint, "endpoint", "GitHub's API endpoint") | |||
| flag.StringVar(&o.graphqlEndpoint, "graphql-endpoint", github.DefaultGraphQLEndpoint, "GitHub's GraphQL API Endpoint") | |||
| flag.StringVar(&o.token, "token", "", "Path to github token") | |||
There was a problem hiding this comment.
Could we keep these temporarily and backfill o.github.* items from them to avoid breaking existing users (outside of k8s)?
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.
| - --token=/etc/github-token/token | ||
| - --endpoint=http://ghproxy.default.svc.cluster.local | ||
| - --github-token=/etc/github-token/token | ||
| - --github-endpoint=http://ghproxy.default.svc.cluster.local |
There was a problem hiding this comment.
this isn't a sufficiently safe way to make this change, as we're not running from source here, see the image above
| log.Printf("Searching: %s", query) | ||
| issues, err := c.FindIssues(query, sort, asc) | ||
| 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 org:foo or repo:foo/bar
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:
a) you have to parse the query
b) the org can be implied be any of the repo: specs with the full name using /
c) with a) and b) together it would become complex and not intuitive (the error message can't just say use a single org: because of b) )
There was a problem hiding this comment.
I don't think its very complex, you just have to extract all org: and repo: blocks and for the repo: blocks then extract the org. If overall you get exactly one org, use that. If not, error out mentioning that you got too many or none. This could also be directly in the github client method to get rid of the mandatory org arg.
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
This PR adds GitHub App authentication for
commenterrobot and unifies the flags and client building with prow machinery. Unfortunately, this requires changing the flag names (mostly prefixed bygithub-, so I've updated the jobs as well.Resolves #32805