Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for EasyCLA #22742

Closed
wants to merge 11 commits into from
Closed

Add support for EasyCLA #22742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
856aeda
Add support for EasyCLA
anusha94 Jun 30, 2021
064bec8
Update handle and handleComment with parameter for easyCLAEnabled
anusha94 Jul 9, 2021
4fd9dbd
Fixed formatting errors
anusha94 Jul 9, 2021
1125ef4
Added a new CLAConfig struct and negative test cases
anusha94 Jul 10, 2021
9951f53
Updating test fixtures for prow cla plugin
anusha94 Jul 11, 2021
13e6fe9
Updating bazel rules and dependencies
anusha94 Jul 11, 2021
d1d3fc2
Add default values for CLAContextNames
anusha94 Jul 14, 2021
d05897a
Adding default CLAConfig value in plugins_test
anusha94 Jul 18, 2021
d998079
Added documentation for CLAContextNames field
anusha94 Jul 24, 2021
f427fba
Modify TestCheckCLA to include multiple conflicting contexts
anusha94 Aug 2, 2021
7b7bc3d
Update formatting for cla_test.go
anusha94 Aug 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions prow/plugins/cla/BUILD.bazel
View file
Open in desktop
Expand Up @@ -14,6 +14,7 @@ go_test(
"//prow/github:go_default_library",
"//prow/github/fakegithub:go_default_library",
"//prow/labels:go_default_library",
"//prow/plugins:go_default_library",
"@com_github_sirupsen_logrus//:go_default_library",
],
)
Expand Down
32 changes: 19 additions & 13 deletions prow/plugins/cla/cla.go
View file
Open in desktop
Expand Up @@ -33,7 +33,6 @@ import (

const (
pluginName = "cla"
claContextName = "cla/linuxfoundation"
cncfclaNotFoundMessage = `Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: **Please follow instructions at <https://git.k8s.io/community/CLA.md#the-contributor-license-agreement> to sign the CLA.**
Expand Down Expand Up @@ -70,7 +69,7 @@ func helpProvider(config *plugins.Configuration, _ []config.OrgRepo) (*pluginhel
// The {WhoCanUse, Usage, Examples, Config} fields are omitted because this plugin cannot be
// manually triggered and is not configurable.
pluginHelp := &pluginhelp.PluginHelp{
Description: "The cla plugin manages the application and removal of the 'cncf-cla' prefixed labels on pull requests as a reaction to the " + claContextName + " github status context. It is also responsible for warning unauthorized PR authors that they need to sign the CNCF CLA before their PR will be merged.",
Description: "The cla plugin manages the application and removal of the 'cncf-cla' prefixed labels on pull requests as a reaction to the EasyCLA / cla-linuxfoundation github status context. It is also responsible for warning unauthorized PR authors that they need to sign the CNCF CLA before their PR will be merged.",
}
pluginHelp.AddCommand(pluginhelp.Command{
Usage: "/check-cla",
Expand All @@ -93,20 +92,20 @@ type gitHubClient interface {
}

func handleStatusEvent(pc plugins.Agent, se github.StatusEvent) error {
return handle(pc.GitHubClient, pc.Logger, se)
return handle(pc.GitHubClient, pc.Logger, se, pc.PluginConfig.CLAConfig)
}

// 1. Check that the status event received from the webhook is for the CNCF-CLA.
// 2. Use the github search API to search for the PRs which match the commit hash corresponding to the status event.
// 3. For each issue that matches, check that the PR's HEAD commit hash against the commit hash for which the status
// was received. This is because we only care about the status associated with the last (latest) commit in a PR.
// 4. Set the corresponding CLA label if needed.
anusha94 marked this conversation as resolved.
Show resolved Hide resolved
func handle(gc gitHubClient, log *logrus.Entry, se github.StatusEvent) error {
func handle(gc gitHubClient, log *logrus.Entry, se github.StatusEvent, cc plugins.CLAConfig) error {
if se.State == "" || se.Context == "" {
return fmt.Errorf("invalid status event delivered with empty state/context")
}

if se.Context != claContextName {
if !contains(cc.CLAContextNames, se.Context) {
// Not the CNCF CLA context, do not process this.
return nil
}
Expand Down Expand Up @@ -193,10 +192,10 @@ func handle(gc gitHubClient, log *logrus.Entry, se github.StatusEvent) error {
}

func handleCommentEvent(pc plugins.Agent, ce github.GenericCommentEvent) error {
return handleComment(pc.GitHubClient, pc.Logger, &ce)
return handleComment(pc.GitHubClient, pc.Logger, &ce, pc.PluginConfig.CLAConfig)
}

func handleComment(gc gitHubClient, log *logrus.Entry, e *github.GenericCommentEvent) error {
func handleComment(gc gitHubClient, log *logrus.Entry, e *github.GenericCommentEvent, cc plugins.CLAConfig) error {
// Only consider open PRs and new comments.
if e.IssueState != "open" || e.Action != github.GenericCommentActionCreated {
return nil
Expand Down Expand Up @@ -240,9 +239,8 @@ func handleComment(gc gitHubClient, log *logrus.Entry, e *github.GenericCommentE
}

for _, status := range combined.Statuses {

// Only consider "cla/linuxfoundation" status.
if status.Context == claContextName {
// Only consider "cla/linuxfoundation" or "EasyCLA" status
if contains(cc.CLAContextNames, status.Context) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we know if combined.Statuses is sorted by the time we get here?


// Success state implies that the cla exists, so label should be cncf-cla:yes.
if status.State == github.StatusSuccess {
Expand Down Expand Up @@ -278,10 +276,18 @@ func handleComment(gc gitHubClient, log *logrus.Entry, e *github.GenericCommentE
}
}
}

// No need to consider other contexts once you find the one you need.
break
Comment on lines -281 to -283
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there's no break anymore, now we are potentially adding/removing labels twice.

What I asked for in last review was to make the decision before acting, e.g.

claState := github.StatusPending
for _, status := range combined.Statuses {
  // code that sets claState
}
// code that sets labels based on claState

}
}
return nil
}

// contains checks if a string is present in a slice
func contains(s []string, str string) bool {
for _, v := range s {
if v == str {
return true
}
}

return false
}
160 changes: 123 additions & 37 deletions prow/plugins/cla/cla_test.go
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@ import (
"k8s.io/test-infra/prow/github"
"k8s.io/test-infra/prow/github/fakegithub"
"k8s.io/test-infra/prow/labels"
"k8s.io/test-infra/prow/plugins"
)

func TestCLALabels(t *testing.T) {
Expand Down Expand Up @@ -140,6 +141,34 @@ func TestCLALabels(t *testing.T) {
addedLabels: []string{fmt.Sprintf("/#3:%s", labels.ClaNo)},
removedLabels: []string{fmt.Sprintf("/#3:%s", labels.ClaYes)},
},
{
name: "EasyCLA status failure removes \"cncf-cla: yes\" label",
context: "EasyCLA",
anusha94 marked this conversation as resolved.
Show resolved Hide resolved
state: "failure",
Comment on lines +146 to +147
Copy link
Member

@spiffxp spiffxp Jul 20, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the plugin now supports multiple contexts, I think it's fair to ask the test cases be written to test multiple contexts (or that you add a new Test that supports this).

I would like to see test cases with multiple contexts having conflicting values, so we can confirm that it's first-context-wins in all cases

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. I would like to see something like

contexts:
  "cla/linuxfoundation": "success" 
  "EasyCLA": "failure"

and vice-versa

statusSHA: "a",
issues: []github.Issue{
{Number: 3, State: "open", Labels: []github.Label{{Name: labels.ClaYes}}},
},
pullRequests: []github.PullRequest{
{Number: 3, Head: github.PullRequestBranch{SHA: "a"}},
},
addedLabels: []string{fmt.Sprintf("/#3:%s", labels.ClaNo)},
removedLabels: []string{fmt.Sprintf("/#3:%s", labels.ClaYes)},
},
{
name: "EasyCLA status success removes \"cncf-cla: no\" label",
context: "EasyCLA",
state: "success",
statusSHA: "a",
issues: []github.Issue{
{Number: 3, State: "open", Labels: []github.Label{{Name: labels.ClaNo}}},
},
pullRequests: []github.PullRequest{
{Number: 3, Head: github.PullRequestBranch{SHA: "a"}},
},
addedLabels: []string{fmt.Sprintf("/#3:%s", labels.ClaYes)},
removedLabels: []string{fmt.Sprintf("/#3:%s", labels.ClaNo)},
},
}
for _, tc := range testcases {
pullRequests := make(map[int]*github.PullRequest)
Expand All @@ -161,7 +190,10 @@ func TestCLALabels(t *testing.T) {
SHA: tc.statusSHA,
State: tc.state,
}
if err := handle(fc, logrus.WithField("plugin", pluginName), se); err != nil {
cc := plugins.CLAConfig{
CLAContextNames: []string{"cla/linuxfoundation", "EasyCLA"},
}
if err := handle(fc, logrus.WithField("plugin", pluginName), se, cc); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think I assumed there was less duplication of logic before

So the way I read the StatusEvent handling code, is that it will respond to whatever the status is. So if the CLA plugin is configured to listen to two CLA statuses, we will have a race between which of the two statuses reports in first. Slowest status wins.

I think what would provide more consistency is to try to de-dupe the logic and do what handleComment does: get the PR's combined statuses, and ensure one of them consistently wins the race.

e.g. let's say cla/linuxfoundation consistently wins:

# happy path
state: 
  cla/linuxfoundation: pending
  EasyCLA: pending
  labels: []
receive:
  cla/linuxfoundation: success
result:
  add: "cncf-cla: yes"

# conflict: EasyCLA arrives last
state: 
  cla/linuxfoundation: success
  EasyCLA: pending
  labels: ["cncf-cla: yes"]
receive:
  EasyCLA: failure
result:
  # do nothing

# conflict: cla/linuxfoundation arrives last
state: 
  cla/linuxfoundation: pending
  EasyCLA: success
  labels: ["cncf-cla: yes"]
receive:
  cla/linuxfoundation: failure
result:
  add: "cncf-cla: no"
  remove: "cncf-cla: yes"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if the CLA plugin is configured to listen to two CLA statuses

From the docs, The most recent status for each context is returned.
I think my question is - are EasyCLA and cla/linuxfoundation considered as two different contexts or different names for a single CLA context? How does getting combined status here work? Combined status is still going to return both the contexts - how do we know which one is the latest?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my question is - are EasyCLA and cla/linuxfoundation considered as two different contexts or different names for a single CLA context

Well, I'm assuming I'm going to see two entries, one called cla/linuxfoundation and one called EasyCLA while the CNCF has both things running simultaneously on their end. So they're different contexts. You're updating the plugin to decide which one to treat as the authoritative source of truth from the CNCF. This allows us to have the plugin pay attention to the new EasyCLA check, but defer to the old cla/linuxfoundation check as authoritative, without having to change anything.

Another possibly simpler option is to roll back some of what you've done, and update the plugin to listen to one-and-only-one context. In this scenario we would:

  • make the plugin listen to a configurable context
  • start with cla/linuxfoundation as this plugin's source of truth
  • setup EasyCLA on the CNCF's end to start reporting to PRs
  • setup prow/branch-protector to treat EasyCLA contexts as non-blocking
  • wait
  • generate a report that shows over time, for all PRs within a given window, PRs where cla/linuxfoundation and EasyCLA differed
  • figure out why and fix
  • wait again
  • generate report again
  • repeat until there is no difference, and then we change branch-protector to make the EasyCLA context required, the cla/linuxfoundation context non-blocking, and the cla plugin to listen to EasyCLA to apply "cncf-cla: yes" labels

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does getting combined status here work? Combined status is still going to return both the contexts - how do we know which one is the latest?

What I was saying above is that the handle function doesn't call GetCombinedStatus, it only consider the status from the StatusEvent it's handling. So it will get called once whenever cla/linuxfoundation is update, and again whenever EasyCLA is updated, and there's no way to guarantee which of those is going to arrive first.

Calling GetCombinedStatus to get both contexts is the point. We don't care which one came in latest, we only care that it came in for the commit we're considering, and that the authoritative context (if set) always overrides the others.

Again, I'm thinking this may all be too confusing, and it might be simpler to support a single (configurable) context only. But we need to see how EasyCLA behaves live (in terms of responsiveness, reliability, consistency, and agreement with cla/linuxfoundation) before I will ever consider changing our workflow to pay attention to that over cla/linuxfoundation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EasyCLA has been enabled for the k/org repo in a non-blocking way as a test.
For some additional information, EasyCLA is currently in use by 3 other CNCF projects:

  • Open telemetry (47 repos)
  • grpc (26 repos)
  • cloud custodian (6 repos)

For gRPC it's been some time (most of the kinks in the project were worked out with this one), I don't know when it was enabled for Otel or cloud custodian.

The LF has said if we do have problems we can roll back.

t.Errorf("For case %s, didn't expect error from cla plugin: %v", tc.name, err)
continue
}
Expand All @@ -179,8 +211,7 @@ func TestCLALabels(t *testing.T) {
func TestCheckCLA(t *testing.T) {
var testcases = []struct {
name string
context string
state string
contexts map[string]string
issueState string
SHA string
action string
Expand All @@ -193,9 +224,10 @@ func TestCheckCLA(t *testing.T) {
removedLabel string
}{
{
name: "ignore non cla/linuxfoundation context",
context: "random/context",
state: "success",
name: "ignore non cla/linuxfoundation context",
contexts: map[string]string{
"random/context": "success",
},
issueState: "open",
SHA: "sha",
action: "created",
Expand All @@ -205,9 +237,10 @@ func TestCheckCLA(t *testing.T) {
},
},
{
name: "ignore non open PRs",
context: "cla/linuxfoundation",
state: "success",
name: "ignore non open PRs",
contexts: map[string]string{
"cla/linuxfoundation": "success",
},
issueState: "closed",
SHA: "sha",
action: "created",
Expand All @@ -217,9 +250,10 @@ func TestCheckCLA(t *testing.T) {
},
},
{
name: "ignore non /check-cla comments",
context: "cla/linuxfoundation",
state: "success",
name: "ignore non /check-cla comments",
contexts: map[string]string{
"cla/linuxfoundation": "success",
},
issueState: "open",
SHA: "sha",
action: "created",
Expand All @@ -229,9 +263,10 @@ func TestCheckCLA(t *testing.T) {
},
},
{
name: "do nothing on when status state is \"pending\"",
context: "cla/linuxfoundation",
state: "pending",
name: "do nothing on when status state is \"pending\"",
contexts: map[string]string{
"cla/linuxfoundation": "pending",
},
issueState: "open",
SHA: "sha",
action: "created",
Expand All @@ -241,9 +276,10 @@ func TestCheckCLA(t *testing.T) {
},
},
{
name: "cla/linuxfoundation status adds the cla-yes label when its state is \"success\"",
context: "cla/linuxfoundation",
state: "success",
name: "EasyCLA status adds the cla-yes label when its state is \"success\"",
contexts: map[string]string{
"EasyCLA": "success",
},
issueState: "open",
SHA: "sha",
action: "created",
Expand All @@ -255,9 +291,10 @@ func TestCheckCLA(t *testing.T) {
addedLabel: fmt.Sprintf("/#3:%s", labels.ClaYes),
},
{
name: "cla/linuxfoundation status adds the cla-yes label and removes cla-no label when its state is \"success\"",
context: "cla/linuxfoundation",
state: "success",
name: "EasyCLA status adds the cla-yes label and removes cla-no label when its state is \"success\"",
contexts: map[string]string{
"EasyCLA": "success",
},
issueState: "open",
SHA: "sha",
action: "created",
Expand All @@ -271,9 +308,10 @@ func TestCheckCLA(t *testing.T) {
removedLabel: fmt.Sprintf("/#3:%s", labels.ClaNo),
},
{
name: "cla/linuxfoundation status adds the cla-no label when its state is \"failure\"",
context: "cla/linuxfoundation",
state: "failure",
name: "cla/linuxfoundation status adds the cla-no label when its state is \"failure\"",
contexts: map[string]string{
"cla/linuxfoundation": "failure",
},
issueState: "open",
SHA: "sha",
action: "created",
Expand All @@ -285,9 +323,10 @@ func TestCheckCLA(t *testing.T) {
addedLabel: fmt.Sprintf("/#3:%s", labels.ClaNo),
},
{
name: "cla/linuxfoundation status adds the cla-no label and removes cla-yes label when its state is \"failure\"",
context: "cla/linuxfoundation",
state: "failure",
name: "EasyCLA status adds the cla-no label and removes cla-yes label when its state is \"failure\"",
contexts: map[string]string{
"EasyCLA": "failure",
},
issueState: "open",
SHA: "sha",
action: "created",
Expand All @@ -301,9 +340,10 @@ func TestCheckCLA(t *testing.T) {
removedLabel: fmt.Sprintf("/#3:%s", labels.ClaYes),
},
{
name: "cla/linuxfoundation status retains the cla-yes label and removes cla-no label when its state is \"success\"",
context: "cla/linuxfoundation",
state: "success",
name: "cla/linuxfoundation status retains the cla-yes label and removes cla-no label when its state is \"success\"",
contexts: map[string]string{
"cla/linuxfoundation": "success",
},
issueState: "open",
SHA: "sha",
action: "created",
Expand All @@ -317,9 +357,10 @@ func TestCheckCLA(t *testing.T) {
removedLabel: fmt.Sprintf("/#3:%s", labels.ClaNo),
},
{
name: "cla/linuxfoundation status retains the cla-no label and removes cla-yes label when its state is \"failure\"",
context: "cla/linuxfoundation",
state: "failure",
name: "cla/linuxfoundation status retains the cla-no label and removes cla-yes label when its state is \"failure\"",
contexts: map[string]string{
"cla/linuxfoundation": "failure",
},
issueState: "open",
SHA: "sha",
action: "created",
Expand All @@ -332,6 +373,42 @@ func TestCheckCLA(t *testing.T) {

removedLabel: fmt.Sprintf("/#3:%s", labels.ClaYes),
},
{
name: "cla/linuxfoundation status retains the cla-no label and removes cla-yes label when its state is \"failure\"",
contexts: map[string]string{
"cla/linuxfoundation": "failure",
"EasyCLA": "success",
},
Comment on lines +378 to +381
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the refactor down here. I think I'm asking for a similar refactor to the status event handling test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. will do :)

issueState: "open",
SHA: "sha",
action: "created",
body: "/check-cla",
pullRequests: []github.PullRequest{
{Number: 3, Head: github.PullRequestBranch{SHA: "sha"}},
},
hasCLANo: true,
hasCLAYes: true,

removedLabel: fmt.Sprintf("/#3:%s", labels.ClaYes),
},
{
name: "cla/linuxfoundation status retains the cla-yes label and removes cla-no label when its state is \"success\"",
contexts: map[string]string{
"EasyCLA": "failure",
"cla/linuxfoundation": "success",
},
issueState: "open",
SHA: "sha",
action: "created",
body: "/check-cla",
pullRequests: []github.PullRequest{
{Number: 3, Head: github.PullRequestBranch{SHA: "sha"}},
},
hasCLANo: true,
hasCLAYes: true,

removedLabel: fmt.Sprintf("/#3:%s", labels.ClaNo),
},
}
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
Expand All @@ -348,11 +425,20 @@ func TestCheckCLA(t *testing.T) {
Number: 3,
IssueState: tc.issueState,
}
cc := plugins.CLAConfig{
CLAContextNames: []string{"cla/linuxfoundation", "EasyCLA"},
}
var statuses []github.Status
for context, state := range tc.contexts {
statuses = append(statuses, github.Status{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: my "are the statuses sorted" comment above, your unit tests are relying on this being the sort

what I lack is proof that you've verified this sort is what you actually see in production (show me code that presorts the statuses, or docs from github guaranteeing the order in which they deliver statuses)

Copy link
Contributor Author

@anusha94 anusha94 Aug 4, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spiffxp

I hear you. But I could not find proof for sort order. Here is the docs for getting combined status
and the client code to get the same

Any pointers on how we can validate the order?

Copy link
Member

@spiffxp spiffxp Aug 4, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best way to ensure something is sorted is to sort it. So, sort the contexts before you iterate over them. Or, flip the loop so that you iterate over the context names that you care about in the order you care about, and then see if they're contained in combined statuses

State: state,
Context: context,
})
}

fc.CombinedStatuses = map[string]*github.CombinedStatus{
tc.SHA: {
Statuses: []github.Status{
{State: tc.state, Context: tc.context},
},
Statuses: statuses,
},
}
if tc.hasCLAYes {
Expand All @@ -361,7 +447,7 @@ func TestCheckCLA(t *testing.T) {
if tc.hasCLANo {
fc.IssueLabelsAdded = append(fc.IssueLabelsAdded, fmt.Sprintf("/#3:%s", labels.ClaNo))
}
if err := handleComment(fc, logrus.WithField("plugin", pluginName), e); err != nil {
if err := handleComment(fc, logrus.WithField("plugin", pluginName), e, cc); err != nil {
t.Errorf("For case %s, didn't expect error from cla plugin: %v", tc.name, err)
}
ok := tc.addedLabel == ""
Expand Down