Plugin to add issue and PR to GitHub Project #10982

taragu · 2019-01-27T15:39:20Z

Fixes #10514
Need to wait for PR #10401 to be merged

/kind feature

TODO:

add fake client methods in prow/github/fakegithub/fakegithub.go and unit tests (after PR WIP: Auto add PRs to project if they match a search query #10401 is merged)

prow/github/client.go

stevekuznetsov

Great start here! I think there's still some opportunity to hash out the user experience (/cc @cblecker) and GitHub token usage (/cc @cjwagner)

prow/github/client.go

stevekuznetsov · 2019-01-27T22:57:58Z

prow/plugins.yaml

@@ -222,6 +222,15 @@ repo_milestone:
    maintainers_id: 2460384
    maintainers_team: kubernetes-milestone-maintainers

+repo_project:
+  # Default maintainer for projects
+  '':


What does this mean?

We use '*' in other plugins to denote the default. Would be good to either:

match that here

match the structure of the branch protection config here for hierarchical data with overrides

@cjwagner @fejta please let me know which direction you'd rather see.

@stevekuznetsov for approach 2, does something like this work?

project: - orgs: - kubernetes: # Org level config, which can be overriden by repo level configs under repos maintainers_team_id: 2460384 maintainers_team_name: kubernetes-milestone-maintainers columns: - To do - To triage - Backlog - repos: - kubernetes: maintainers_team_id: 2460384 maintainers_team_name: kubernetes-milestone-maintainers columns: - To do - To triage - Backlog - kubeflow: maintainers_team_id: 2460384 maintainers_team_name: kubernetes-milestone-maintainers columns: - To do - To triage - Backlog

or something like this? i think this might be more readable:

project: # Org level config, which can be overriden by repo level configs under repos kubernetes/*: - maintainer_team_id: 2460384 maintainers_team_name: kubernetes-milestone-maintainers columns: - To do - To triage - Backlog kubernetes/kubernetes: - maintainer_team_id: 2460384 maintainers_team_name: kubernetes-milestone-maintainers columns: - To do - To triage - Backlog

I think your first mockup is more true to the branchprotection config and I would rather see that one, but don't let me be the only opinion here!

@stevekuznetsov I see. I prefer the second one because it's easier to read IMHO

Totally. @fejta thoughts? Another data point is (unfortunately) we have momentum behind the approach that already exists in our config, and keeping things similar would help everyone keep it straight

@stevekuznetsov yeah I see. It makes sense to keep it consistent with existing configs

stevekuznetsov · 2019-01-27T22:58:20Z

prow/plugins/config.go

+	Label                      *Label                   `json:"label,omitempty"`
+	Lgtm                       []Lgtm                   `json:"lgtm,omitempty"`
+	RepoMilestone              map[string]Milestone     `json:"repo_milestone,omitempty"`
+	ProjectConfigMap           map[string]ProjectConfig `json:"repo_project,omitempty"`


Mapped by org/repo?

stevekuznetsov · 2019-01-27T22:58:49Z

prow/plugins/config.go

@@ -372,6 +373,16 @@ type Milestone struct {
 	MaintainersTeam string `json:"maintainers_team,omitempty"`
 }

+// ProjectConfig contains the configuration options for the project plugin
+type ProjectConfig struct {


Is this a good config to emulate the branch-protection-style tree overrides?

stevekuznetsov · 2019-01-27T22:59:31Z

prow/plugins/project/project.go

+		Usage:       "/project <board>, /project <board> <column>, or /project clear <board>",
+		Description: "Updates the project board and column for an issue or PR",
+		Featured:    false,
+		WhoCanUse:   "Members of the project maintainers GitHub team can use the '/project' command.",


Can this team be called out here with the config value?

Outstanding

@stevekuznetsov i'm not sure what's the best way to call out the team value here, because I don't have access to the org and repo information

stevekuznetsov · 2019-01-27T23:05:43Z

prow/plugins/project/project.go

+	}
+
+	// Get all projects for this org
+	orgProjects, err := gc.GetOrgProjects(org)


We should only make this call if we don't find the project we need in the repo

stevekuznetsov · 2019-01-27T23:06:03Z

prow/plugins/project/project.go

+	projectMap := buildProjectMap(projects)
+	projectID, ok := projectMap[proposedProject]
+
+	if proposedProject == clearKeyword {


If this is the case, we shouldn't load project columns from GitHub

stevekuznetsov · 2019-01-27T23:06:28Z

prow/plugins/project/project.go

+		projectToClear := matches[2]
+		projectToClearID, ok := projectMap[projectToClear]
+		if ok {
+			existingProjectCard, err := gc.GetProjectCard(projectToClearID, e.Number)


Why do we need his call if we already have the ID?

We have the project ID but we need to project card ID in order to move the project card

stevekuznetsov · 2019-01-27T23:06:57Z

prow/plugins/project/project.go

+		}
+	}
+
+	// If proposedColumn is not found (or not provided), add to the "To do",


Maybe we should expose the default column as a config option and do nothing if it is not set?

stevekuznetsov · 2019-01-27T23:07:24Z

prow/plugins/project/project.go

+	// Move this issue/PR to the new column if there's already a project card for
+	// this issue/PR in this project
+	existingProjectCard, err := gc.GetProjectCard(projectID, e.Number)
+	if err != nil {


Will you get a not found error here?

Yeah probably will, in which case I shouldn't check for err here because we will move onto creating a new project card

fejta · 2019-01-28T08:03:19Z

please pass tests

fejta · 2019-02-11T18:54:03Z

Dependent PR seems stalled. Asked for an ETA on it.

stevekuznetsov · 2019-02-11T18:56:52Z

I think it's fine to just commit the client changes in this PR and not depend on the other.

stevekuznetsov

This is looking really good! Some nits, some comments on how often we post back to GitHub

stevekuznetsov · 2019-02-11T18:57:49Z

prow/plugins.yaml

@@ -222,6 +222,15 @@ repo_milestone:
    maintainers_id: 2460384
    maintainers_team: kubernetes-milestone-maintainers

+repo_project:
+  # Default maintainer for projects
+  '':


We use '*' in other plugins to denote the default. Would be good to either:

match that here

match the structure of the branch protection config here for hierarchical data with overrides

@cjwagner @fejta please let me know which direction you'd rather see.

stevekuznetsov · 2019-02-11T18:59:49Z

prow/plugins/config.go

+	ProjectMaintainersTeam string `json:"project_maintainers_team,omitempty"`
+
+	// If no column is provided when adding an issue/PR to a project, the issue/PR
+	// will be added to one of these columns (in the order they are specified)


Wait, so when does it add to the second column in this list?

@stevekuznetsov if the first column in the default list (To do) doesn't exist in the project, then it'll look for the second column (To triage), and then the third Backlog

I think this might be confusing or surprising behavior for users. @cblecker

stevekuznetsov · 2019-02-11T19:00:10Z

prow/plugins/project/project.go

+		Usage:       "/project <board>, /project <board> <column>, or /project clear <board>",
+		Description: "Updates the project board and column for an issue or PR",
+		Featured:    false,
+		WhoCanUse:   "Members of the project maintainers GitHub team can use the '/project' command.",


Outstanding

stevekuznetsov · 2019-02-11T19:00:23Z

prow/plugins/project/project.go

+	}
+
+	pluginHelp := &pluginhelp.PluginHelp{
+		Description: "The project plugin allows members of a configurable GitHub team to set the project and column on an issue or pull request.",


s/configurable //

stevekuznetsov · 2019-02-11T19:01:03Z

prow/plugins/project/project.go

+	pluginHelp := &pluginhelp.PluginHelp{
+		Description: "The project plugin allows members of a configurable GitHub team to set the project and column on an issue or pull request.",
+		Config: func(repos []string) map[string]string {
+			configMap := make(map[string]string)


I think we generally prefer

configMap := map[string]string{}

When no size is required

stevekuznetsov · 2019-02-11T19:07:22Z

prow/plugins/project/project.go

+		repoProjects, err := gc.GetRepoProjects(org, repo.Name)
+		if err != nil {
+			log.WithError(err).Errorf("Error listing all projects under repo %s", repo.Name)
+			return err


In these cases we should report some internal error to the user on GitHub and ask them to re-try their command

stevekuznetsov · 2019-02-11T19:10:47Z

prow/plugins/project/project.go

+	invalidNumArgs     = "Please provide 2 or 3 arguments. Example usage: /project 0.5.0, /project 0.5.0 3999801, /project clear 0.4.0"
+	projectTeamMsg     = "The project maintainers team is the Github team %q with ID: %d."
+	clearKeyword       = "clear"
+	projectNameToIDMap map[string]int


Let's not use a global here, but we would need a project client similar to that which the other plugins have like ownersClient or slack client. Alternatively if we don't think it is necessary to have an in-memory cache in this plugin for projects, we can just forget about it and offload the caching to the ghproxy. Sorry, I know I brought this up in the first place, but removing this might be the best approach for the first impl

@stevekuznetsov I see. I think it might be overkill to have a client in this case. Could you point me to an example of how to offload the caching to ghproxy?

Using ghproxy will be invisible to this code -- we just pass the proxy as the URL to GitHub when we start up the binary. So, to use it we can just ignore the fact in the code and keep things simple here.

stevekuznetsov · 2019-02-11T19:12:42Z

prow/plugins/project/project.go

+	if proposedProject == clearKeyword {
+		if err := gc.DeleteProjectCard(projectID); err != nil {
+			log.WithError(err).Errorf("Error removing project card from project %s (ID %d) for %s/%s#%d.", projectToFind, projectID, org, repo, e.Number)
+			return err


This should be shown to the user?

stevekuznetsov · 2019-02-11T19:12:59Z

prow/plugins/project/project.go

+			log.WithError(err).Errorf("Error removing project card from project %s (ID %d) for %s/%s#%d.", projectToFind, projectID, org, repo, e.Number)
+			return err
+		}
+		return nil


Should we report a success to the user?

stevekuznetsov · 2019-02-11T19:13:29Z

prow/plugins/project/project.go

+		return err
+	}
+
+	// If proposedColumn is not found (or not provided), add to one of the default


This should be documented in the configuration fields

@cblecker is this surprising behavior? Should we just do the simple thing and error if we can't find the column that was requested?

stevekuznetsov · 2019-02-11T19:15:12Z

Really would love @kubernetes/sig-contributor-experience-pr-reviews (@cblecker) to look at this with @cjwagner -- the impl is very close, but the UX and workflow should be OK'ed first.

taragu · 2019-03-01T20:42:11Z

/test pull-test-infra-lint

stevekuznetsov

This is looking really nice. I think soon we should merge it, turn it on for k/test-infra and test it out. That being said, it's also getting large enough to be hard to review

@cblecker @cjwagner

stevekuznetsov · 2019-03-02T00:41:58Z

prow/plugins.yaml

+project_config:
+  project_org_configs:
+    kubernetes:
+      org_maintainers_team_id: 2460384


nit: could be

org_maintainers_team: id: name:

With this, it looks like you could override the team ID but not the team name with a repo config? Is that something we want?

Yes that's true. When checking a user against the team only the team ID is used. I will remove the team name from the config

stevekuznetsov · 2019-03-02T00:42:11Z

prow/plugins.yaml

+          To do
+      project_repo_configs:
+        kubernetes:
+          repo_maintainers_team_id: 2460384


Do we need to specify these? They are the same as in the owning org

It's probably a bad example for me to use the same team here. They could be different and the repo setting will override the org setting

stevekuznetsov · 2019-03-02T00:43:33Z

prow/plugins/project/project.go

+	plugins.RegisterGenericCommentHandler(pluginName, handleGenericComment, helpProvider)
+}
+
+func getMaintainerTeam(pluginConfigs plugins.ProjectConfig, org string, repo string) (int, string) {


Nit: this could be a method on ProjectConfig

stevekuznetsov · 2019-03-02T00:43:43Z

prow/plugins/project/project.go

+	return 0, ""
+}
+
+func getColumnMap(pluginConfigs plugins.ProjectConfig, org string, repo string) map[string]string {


Nit: this could be a method on ProjectConfig

stevekuznetsov · 2019-03-02T00:44:18Z

prow/plugins/project/project.go

+	return nil
+}
+
+func getOrgColumnMap(pluginConfigs plugins.ProjectConfig, org string, repo string) map[string]string {


Nit: this could be a method on ProjectConfig and does not need the repo parameter

stevekuznetsov · 2019-03-02T00:51:07Z

prow/plugins/project/project.go

+	repos, err := gc.GetRepos(org, false)
+	if err != nil {
+		msg = fmt.Sprintf(errListingReposMsg, org)
+		return gc.CreateComment(org, repo, e.Number, plugins.FormatResponseRaw(e.Body, e.HTMLURL, e.User.Login, msg))


So some of these errors are useful to users -- like if they try to use this command with a project that doesn't exist, or if they're not in the group. However, this call will fail only if there is an infrastructure error -- if we don't have permissions or can't hit the API or something. We should just return the error here

stevekuznetsov · 2019-03-02T00:53:43Z

prow/plugins/project/project.go

+		// specified in the project config and see if any of them exists on the
+		// proposed project
+		if proposedColumnName == "" {
+			defaultColumn := ""


Similar comment here, you could just call getColumnMap and not need explicit default values

stevekuznetsov · 2019-03-02T00:54:51Z

prow/plugins/project/project.go

+
+	// Move this issue/PR to the new column if there's already a project card for
+	// this issue/PR in this project
+	existingProjectCard, _ := gc.GetColumnProjectCard(proposedColumnID, e.Number)


Why are we ignoring this error?

stevekuznetsov · 2019-03-02T00:56:55Z

prow/plugins/project/project_test.go

+	}
+}
+
+func TestParseCommand(t *testing.T) {


This could use negative test cases

stevekuznetsov · 2019-03-02T00:57:23Z

prow/plugins/project/project_test.go

+		if proposedProject != test.proposedProject ||
+			proposedColumn != test.proposedColumn ||
+			shouldClear != test.shouldClear {
+			t.Errorf("\nFor command %s, expected\n  proposedProject = %s\n  proposedColumn = %s\n  shouldClear = %t\nbut got\n  proposedProject = %s\n  proposedColumn = %s\n  shouldClear = %t\n", test.command, test.proposedProject, test.proposedColumn, test.shouldClear, proposedProject, proposedColumn, shouldClear)


Would be nicer for the person who breaks this test to check them separately and do tailored messaged to each

Or wrap them in a struct and use ObjectReflectDiff or something

taragu · 2019-03-03T13:09:30Z

@stevekuznetsov Just realized the project name can be multiple words, for example a command like /project Deflaking kubernetes e2e tests Inbox (unprioritized) won't be parse properly. Should the project ID (or column ID) be provided in the command instead?

taragu · 2019-03-06T23:14:49Z

Talked to @stevekuznetsov on slack about the multi-word case for project name. We can try out this plugin with the single-word case and then have followup discussion on the command format to handle multi-word project and column names

stevekuznetsov · 2019-03-06T23:59:48Z

@cblecker would love to hear your input on the above ^^
As is this is useful, can we deploy and work on making it better? Hate to see it sit around forever in this PR

stevekuznetsov · 2019-03-13T17:27:51Z

/lgtm
/approve

k8s-ci-robot · 2019-03-13T17:27:55Z

LGTM label has been added.

Git tree hash: 823cb0dd17bf83db215d7a22d83d5a1edb7d3f2c

k8s-ci-robot · 2019-03-13T17:28:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stevekuznetsov, taragu

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~~ [stevekuznetsov]

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

k8s-ci-robot · 2019-03-13T17:43:37Z

@taragu: Updated the plugins configmap in namespace default using the following files:

key plugins.yaml using file prow/plugins.yaml

In response to this:

Fixes #10514
Need to wait for PR #10401 to be merged

/kind feature

TODO:

add fake client methods in prow/github/fakegithub/fakegithub.go and unit tests (after PR WIP: Auto add PRs to project if they match a search query #10401 is merged)

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.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 27, 2019

k8s-ci-robot requested review from fejta and stevekuznetsov January 27, 2019 15:39

k8s-ci-robot added area/prow Issues or PRs related to prow sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 27, 2019

taragu commented Jan 27, 2019

View reviewed changes

prow/github/client.go Show resolved Hide resolved

taragu commented Jan 27, 2019

View reviewed changes

prow/github/client.go Show resolved Hide resolved

taragu commented Jan 27, 2019

View reviewed changes

prow/github/client.go Show resolved Hide resolved

taragu commented Jan 27, 2019

View reviewed changes

prow/github/client.go Show resolved Hide resolved

stevekuznetsov reviewed Jan 27, 2019

View reviewed changes

taragu force-pushed the add-issue-to-project branch 2 times, most recently from adcac06 to 394f49d Compare February 10, 2019 23:35

stevekuznetsov reviewed Feb 11, 2019

View reviewed changes

k8s-ci-robot added the sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. label Feb 11, 2019

taragu force-pushed the add-issue-to-project branch from a2b7729 to 0d701ee Compare February 15, 2019 19:08

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 15, 2019

taragu force-pushed the add-issue-to-project branch 5 times, most recently from a976635 to d9e1dc2 Compare February 15, 2019 23:16

taragu changed the title ~~[WIP] Plugin to add issue and PR to GitHub Project~~ Plugin to add issue and PR to GitHub Project Feb 15, 2019

taragu force-pushed the add-issue-to-project branch from 37baeb4 to 1f80115 Compare March 1, 2019 20:26

stevekuznetsov reviewed Mar 2, 2019

View reviewed changes

taragu force-pushed the add-issue-to-project branch 2 times, most recently from 4f496e8 to 373b6b0 Compare March 6, 2019 23:08

taragu force-pushed the add-issue-to-project branch 2 times, most recently from d947930 to 96ff294 Compare March 7, 2019 18:49

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

taragu added 2 commits March 7, 2019 13:53

Client methods and types for github projects

ac796ab

Plugin to add issue and PR to GitHub Project

60c3fbe

taragu force-pushed the add-issue-to-project branch from 96ff294 to 60c3fbe Compare March 7, 2019 18:55

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

k8s-ci-robot assigned stevekuznetsov Mar 13, 2019

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 13, 2019

k8s-ci-robot merged commit 56ad764 into kubernetes:master Mar 13, 2019

grantr mentioned this pull request Mar 13, 2019

Proposal for planning future releases knative/eventing#888

Closed

This was referenced Mar 13, 2019

Add dummy team for testing prow project plugin kubernetes/org#598

Merged

Dummy Issue: Test Prow project plugin #11766

Closed

Update plugin config for project plugin to use dummy team #11768

Merged

Enable prow project plugin for test-infra #11812

Merged

taragu mentioned this pull request Apr 3, 2019

Add project plugin for test-infra in config file #12037

Merged

taragu mentioned this pull request Apr 20, 2019

Enable project plugin for test-infra #12292

Merged

Plugin to add issue and PR to GitHub Project #10982

Plugin to add issue and PR to GitHub Project #10982

Conversation

taragu commented Jan 27, 2019 • edited

stevekuznetsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taragu Feb 13, 2019 • edited

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

Choose a reason for hiding this comment

taragu Jan 30, 2019 • edited

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

fejta commented Jan 28, 2019

fejta commented Feb 11, 2019

stevekuznetsov commented Feb 11, 2019

stevekuznetsov left a 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

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

stevekuznetsov commented Feb 11, 2019

taragu commented Mar 1, 2019

stevekuznetsov left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taragu commented Mar 3, 2019

taragu commented Mar 6, 2019

stevekuznetsov commented Mar 6, 2019

stevekuznetsov commented Mar 13, 2019

k8s-ci-robot commented Mar 13, 2019

k8s-ci-robot commented Mar 13, 2019

k8s-ci-robot commented Mar 13, 2019

taragu commented Jan 27, 2019 •

edited

taragu Feb 13, 2019 •

edited

taragu Jan 30, 2019 •

edited