Set up GitHub oauth for rerun button #13008

mirandachrist · 2019-06-13T01:00:36Z

More specifically, this change will make it check if the user is logged in when they click the rerun button. If they are, the rerun button will do what it already does and show a popup with a command to run to rerun the test.
If the user is not logged in, it will redirect to GitHub oauth, then redirect back to prow.k8s.io

/assign @cjwagner
/cc @fejta
/cc @Katharine

prow/githuboauth/githuboauth.go

krzyzacy · 2019-06-13T01:13:29Z

high-level design question (maybe for @cjwagner or @Katharine): how would this fit into things like gerrit?

stevekuznetsov · 2019-06-13T16:39:03Z

Why do we want this? The modal popup doesn't do anything other than show you a command you already can't run if you don't have rights.

mirandachrist · 2019-06-13T16:59:10Z

Why do we want this? The modal popup doesn't do anything other than show you a command you already can't run if you don't have rights.

We eventually want to make the button trigger the test directly, and we'll need the GitHub oauth to make sure the user has the rights. This is the first step toward that.

stevekuznetsov · 2019-06-13T17:23:56Z

I don't want this change, though. Today, when people ask me to rerun a job for them they paste me the command and I just run it. This change would make it not possible for them to do that.

mirandachrist · 2019-06-13T17:35:38Z

You'll still be able to rerun jobs for people, since the /rerun endpoint will stay the same, and they can send you the job number, which is viewable from the new YAML button. I could also still show the popup (without oauth required) in addition to giving the option to rerun directly (which would require oauth).
This PR lets anyone with a GitHub account view the popup, so in practice it shouldn't change much.

stevekuznetsov · 2019-06-13T18:08:04Z

I would rather not change the current UX.

What is our final state we're designing towards, here? Are we changing the behavior of /rerun endpoint? Absent a design doc I was hoping we'd see a "rerun" button for jobs that would take you to a page, guarded by OAuth, that would allow you to:

run another job with the same spec
trigger another job by re-resolving the trigger-time spec data

The page could also be used to trigger a job (not necessarily by getting there from an exiting job's view) to allow users to trigger runs of jobs that did not have any current ProwJob records.

IMO we should present the new feature as a standalone item, let it mature, then migrate to it and remove the old button as necessary. This is similar to the approach @Katharine took in implementing spyglass and leads to a smooth transition from the current feature set to the new feature set.

mirandachrist · 2019-06-13T18:10:42Z

See design doc here: https://docs.google.com/document/d/1ykgWeEnh4SwX9AuQz28sicFCpqjuEaYclLVTlStNggg/view#

stevekuznetsov · 2019-06-13T18:18:32Z

Throw those out to the SIG on Slack and the mailing list when you've got them, it's great to read :)

Katharine · 2019-06-13T18:46:20Z

I agree with Steve that I don't think it makes sense to launch parts of this piecemeal into production - the end state is strictly better, but the intermediate states include confusing regressions from current functionality, and shouldn't be what we serve users while we keep working on it.

mirandachrist · 2019-06-13T22:12:05Z

I'll modify this PR so the current popup can still be viewed without oauth, and I'll add a button on the current popup that requires oauth but does nothing (this will turn into the "trigger job directly" button later). Thanks everyone for your input :)

mirandachrist · 2019-06-14T00:50:08Z

It now doesn't require oauth until you click the "Run" button:

After clicking the "Run" button and logging in, you see:

prow/cmd/deck/static/prow/prow.ts

prow/githuboauth/githuboauth.go

prow/githuboauth/githuboauth_test.go

prow/githuboauth/githuboauth.go

alvaroaleman · 2019-06-16T14:56:29Z

I agree with Steve that I don't think it makes sense to launch parts of this piecemeal into production - the end state is strictly better, but the intermediate states include confusing regressions from current functionality, and shouldn't be what we serve users while we keep working on it.

What about featuregating this? That way, @mirandachrist can get something merged without needing the full thing to be done. Additionally, users like us that have a deck instance that is not publicly accessible might want to never activate this feature, because everyone who gets to the point of seeing the button is permitted to use it

Katharine

This looks good from the perspective of doing the authentication.

I'm not sure I love adding a button that explicitly doesn't work - it would be nice to gate that so everyone doesn't see it when they update. I'm also not totally convinced that it should be a link in the already-logged-in case, but I don't think that's the intended final form?

A side note: you've marked a bunch of comments as "resolved", but they haven't been changed. I assume you use this to mean you have made the changes in your working copy. For the sanity of your reviewers, I would request that you not mark something as resolved until you have pushed up the relevant changes.

Katharine · 2019-06-17T19:25:54Z

prow/cmd/deck/static/pr/pr.ts

@@ -387,8 +387,10 @@ function getFullPRContext(builds: Job[], contexts: Context[]): UnifiedContext[]
 */
 function loadPrStatus(prData: UserData): void {
 const tideQueries: TideQuery[] = [];
- for (const query of tideData.TideQueries) {
- tideQueries.push(new TideQuery(query));
+ if (tideData.TideQueries != null) {


Can this happen? According to the definition of TideData, it can't, in which case this check makes no sense.

If you think it can happen, then a) please explain when it happens, and then b) fix the typescript definition of TideData to match. You'll probably subsequently have to make a lot of other changes too.

This fixed a null pointer error that was coming up in my test instance when there were no TideQueries. Honestly, I'm not really sure what was going on. Should I just change it back?

This is probably a real bug that should probably be fixed. I haven't traced the code in depth, but my educated guess is as follows:

Go considers nil to be a valid, empty slice. Consequently, it is idiomatic to initialise a slice with nil (by doing e.g. var someSlice []string or whatever). This nil then eventually gets JSON serialised as null (because it's not really an empty slice, it's just null). While the Go structs do not specify omitempty, and so the empty value is included, the empty value is null rather than an empty array ([]). Unfortunately, Go is unusual in thinking that null is a valid array, and so when we later parse that in TypeScript, it is null with no type and any attempt to operate on it fails.

To fix this, we should either change the TypeScript type so it accepts nulls (and then add null checks probably all over the place), or change the Go code so it can never be nil (e.g. by actually initialising to an empty slice somewhere).

In either case, though, the fix probably belongs in another PR - I'd file an issue, remove this change, and follow up later.

Go is unusual in thinking that null is a valid array

If we're being very pedantic, nil is a valid (zero) value for a slice, but not an array ;)

Added an issue

prow/cmd/deck/static/prow/prow.ts

Katharine · 2019-06-17T19:33:24Z

prow/githuboauth/githuboauth.go

@@ -92,6 +109,8 @@ func NewAgent(config *config.GitHubOAuthConfig, logger *logrus.Entry) *Agent {
 // redirect user to GitHub OAuth end-point for authentication.
 func (ga *Agent) HandleLogin(client OAuthClient) http.HandlerFunc {
 return func(w http.ResponseWriter, r *http.Request) {
+ destPage := r.URL.Query().Get("dest")
+ rerunStatus := r.URL.Query().Get("rerun_status")


This feels very specific to the particular reason we are logging in. Would it make sense to be more generic?

In the future, this will contain the name of the prow job, so when the page reloads after logging in, the same popup for the same job appears. I renamed it to make it make a little more sense.

prow/cmd/deck/static/prow/prow.ts

Katharine · 2019-06-17T19:45:35Z

prow/githuboauth/githuboauth.go

 // Exchanges code from GitHub OAuth redirect for user access token.
 Exchange(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error)
 // Returns a URL to GitHub's OAuth 2.0 consent page. The state is a token to protect the user
 // from an XSRF attack.
 AuthCodeURL(state string, opts ...oauth2.AuthCodeOption) string
 }

+type RealOAuthClient struct {


Why is this "real"?

Got rid of this. It was "real" in contrast to the mock OAuthClient in the tests.

Katharine · 2019-06-17T19:48:41Z

prow/cmd/deck/static/prow/prow.ts

+ if (login === "") {
+ i.href = `https://${window.location.hostname}/github-login?rerun_status=work_in_progress`;
+ } else {
+ runButton.href = `https://${window.location.hostname}/?rerun_status=work_in_progress`;


On further thought, I'm also not a huge fan of this being a link at all, but perhaps that's going to change?

In the future, it will link to the /rerun endpoint and rerun the job, as in Alvaro's PR

Did we decide against a dialog in the page? I haven't checked the design doc recently.

We will have a dialog:

runButton will become the button in the bottom right. After the test is triggered, we'll have another dialog saying it was successful.

Katharine · 2019-06-17T19:52:15Z

/lint

k8s-ci-robot

@Katharine: 5 warnings.

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

prow/githuboauth/githuboauth.go

prow/githuboauth/githuboauth_test.go

stevekuznetsov · 2019-06-17T20:38:25Z

I think these two comments are still outstanding:
#13008 (comment)
#13008 (comment)

Katharine · 2019-06-20T00:15:00Z

I don't think you wanted to include prow.ts.orig

k8s-ci-robot · 2019-06-20T00:30:40Z

@mirandachrist: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-test-infra-gubernator	`8e2923e`	link	`/test pull-test-infra-gubernator`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

mirandachrist · 2019-06-20T00:34:36Z

Removed @Katharine

stevekuznetsov · 2019-06-20T01:28:15Z

You likely want to squash your commits :)

prow/githuboauth/githuboauth.go

mirandachrist · 2019-06-20T20:22:06Z

Done

k8s-ci-robot · 2019-06-20T21:35:07Z

LGTM label has been added.

Git tree hash: 7f73954eb39aa90c502ab0826a5cd7d802ae213f

k8s-ci-robot · 2019-06-20T21:35:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Katharine, mirandachrist, stevekuznetsov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~prow/OWNERS~~ [Katharine,stevekuznetsov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

stevekuznetsov · 2019-06-20T21:49:13Z

/hold cancel

k8s-ci-robot assigned cjwagner Jun 13, 2019

k8s-ci-robot requested review from fejta and Katharine June 13, 2019 01:00

mirandachrist commented Jun 13, 2019

View reviewed changes

prow/githuboauth/githuboauth.go Show resolved Hide resolved

mirandachrist force-pushed the oauth branch from 0d363a9 to 0d38452 Compare June 14, 2019 00:49

stevekuznetsov reviewed Jun 14, 2019

View reviewed changes

Katharine suggested changes Jun 17, 2019

View reviewed changes

k8s-ci-robot assigned Katharine Jun 17, 2019

k8s-ci-robot reviewed Jun 17, 2019

View reviewed changes

mirandachrist force-pushed the oauth branch 2 times, most recently from 64fb313 to 93d5f4a Compare June 17, 2019 20:23

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2019

mirandachrist force-pushed the oauth branch from d67ec86 to 114031b Compare June 19, 2019 22:41

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2019

mirandachrist force-pushed the oauth branch from 114031b to c1bd203 Compare June 19, 2019 23:33

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 19, 2019

mirandachrist force-pushed the oauth branch from 0e82d66 to bbd5b97 Compare June 20, 2019 00:25

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 20, 2019

mirandachrist force-pushed the oauth branch from bbd5b97 to f92fa2d Compare June 20, 2019 00:29

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2019

Katharine reviewed Jun 20, 2019

View reviewed changes

prow/githuboauth/githuboauth.go Outdated Show resolved Hide resolved

Try to fix various git issues

da817f8

mirandachrist force-pushed the oauth branch from d635a79 to da817f8 Compare June 20, 2019 20:19

Katharine approved these changes Jun 20, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 20, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 20, 2019

k8s-ci-robot merged commit d6e1977 into kubernetes:master Jun 20, 2019

Katharine mentioned this pull request Jun 21, 2019

Include host in kubectl command #13117

Merged

mirandachrist mentioned this pull request Jul 3, 2019

Allow direct triggering of reruns #13321

Closed

Set up GitHub oauth for rerun button #13008

Set up GitHub oauth for rerun button #13008

Conversation

mirandachrist commented Jun 13, 2019

krzyzacy commented Jun 13, 2019

stevekuznetsov commented Jun 13, 2019

mirandachrist commented Jun 13, 2019

stevekuznetsov commented Jun 13, 2019 • edited Loading

mirandachrist commented Jun 13, 2019

stevekuznetsov commented Jun 13, 2019

mirandachrist commented Jun 13, 2019

stevekuznetsov commented Jun 13, 2019

Katharine commented Jun 13, 2019

mirandachrist commented Jun 13, 2019

mirandachrist commented Jun 14, 2019

alvaroaleman commented Jun 16, 2019

Katharine left a comment

Choose a reason for hiding this comment

Katharine Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Katharine commented Jun 17, 2019

k8s-ci-robot left a comment

Choose a reason for hiding this comment

stevekuznetsov commented Jun 17, 2019

Katharine commented Jun 20, 2019 • edited Loading

k8s-ci-robot commented Jun 20, 2019 • edited Loading

mirandachrist commented Jun 20, 2019

stevekuznetsov commented Jun 20, 2019

mirandachrist commented Jun 20, 2019

k8s-ci-robot commented Jun 20, 2019

k8s-ci-robot commented Jun 20, 2019

stevekuznetsov commented Jun 20, 2019

stevekuznetsov commented Jun 13, 2019 •

edited

Loading

Katharine Jun 17, 2019 •

edited

Loading

Katharine commented Jun 20, 2019 •

edited

Loading

k8s-ci-robot commented Jun 20, 2019 •

edited

Loading