Add support for EasyCLA #22742

anusha94 · 2021-06-30T14:04:31Z

Added a new field EasyCLAEnabled in the plugins config and based on
this field, if set will assign the claContextName to cla/easy-cla, else
default to cla/linuxfoundation

Fixes #22721

Added a new field `EasyCLAEnabled` in the plugins config and based on this field, if set will assign the claContextName to cla/easy-cla, else default to cla/linuxfoundation

k8s-ci-robot · 2021-06-30T14:04:38Z

Welcome @anusha94!

It looks like this is your first PR to kubernetes/test-infra 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/test-infra has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-06-30T14:04:38Z

Hi @anusha94. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

anusha94 · 2021-06-30T14:12:04Z

/cc @nikhita

anusha94 · 2021-06-30T14:13:30Z

Fixes #22742

MadhavJivrajani · 2021-07-01T07:45:45Z

/ok-to-test

prow/plugins/cla/cla.go

MadhavJivrajani · 2021-07-07T15:53:16Z

Fixes #22721
Edit: looks like you'll have to update it in the PR desc for the issue to auto-close on merge :)

MadhavJivrajani · 2021-07-09T15:35:24Z

/label tide/merge-method-squash

cjwagner

Thanks for the PR!

prow/plugins/config.go

prow/plugins/cla/cla.go

prow/plugins/cla/cla_test.go

k8s-ci-robot · 2021-07-14T12:39:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: anusha94
To complete the pull request process, please ask for approval from cjwagner after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

prow/OWNERS

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

anusha94 · 2021-07-18T09:57:46Z

@cjwagner Could you please review again?

jeefy · 2021-07-20T01:49:04Z

/lgtm

spiffxp

I have concerns over dueling contexts

prow/plugins/config.go

spiffxp · 2021-07-20T23:02:01Z

prow/plugins/cla/cla.go

+		// Valid status values are cla/linuxfoundation and EasyCLA
+		if contains(cc.CLAContextNames, status.Context) {


Can we rewrite this so that it iterates through statuses to determine CLA yes/no first, and then once outside the for loop, applies labels accordingly? I'd like to avoid wasting GitHub tokens on label apply/remove calls while we decide which status wins if they start competing.

Is combined.Statuses sorted at this point? or could we be walking these in random order?

@spiffxp Can you please elaborate on competing statuses? Going by the existing code if status.Context == claContextName, I assumed the for loop should not continue if it is not one of cla/linuxfoundation or EasyCLA

Or are you saying this entire for loop should be rewritten such that applying of ClaNo or ClaYes should be outside the for loop?
Thanks.

Is combined.Statuses sorted at this point? or could we be walking these in random order?

@mrbobbytables Do you know about the sort order of combined.Statuses? Or rather, how can I check / verify the order?

Can you please elaborate on competing statuses?

I missed the break down below. You're right, one of them will win. But will it be the same one every time? And how will I know which one that is?

I removed the break statement so that all contexts will be processed.

prow/plugins/plugin-config-documented.yaml

spiffxp · 2021-07-20T23:06:43Z

prow/plugins/cla/cla_test.go

+			context:   "EasyCLA",
+			state:     "failure",


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

e.g. I would like to see something like

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

and vice-versa

prow/plugins/cla/cla.go

prow/plugins/cla/cla_test.go

anusha94 · 2021-08-02T15:39:26Z

@spiffxp

I removed the break statement from here in order for all the contexts to be processed.

In TestCheckCLA, I changed the context field from string to a map. Does this seem okay? Or having a separate Test for multiple contexts make more sense?

spiffxp

I'm not confident enough to deploy this plugin in a manner that listens to two status contexts simultaneously yet. I wrote down some test cases I would want to see exercised to ensure consistency vs. a race between last-context-reporting-wins.

I can be convinced to allow the EasyCLA check to report to PRs as non-blocking, without this plugin configured to listen to it, to allow us to gather data on how often we encounter conflict between the old CLA check and the new CLA check.

spiffxp · 2021-08-03T18:55:36Z

prow/plugins/cla/cla.go

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


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

spiffxp · 2021-08-03T19:00:16Z

prow/plugins/cla/cla.go

-
-			// No need to consider other contexts once you find the one you need.
-			break


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

spiffxp · 2021-08-03T19:03:37Z

prow/plugins/cla/cla_test.go

+			}
+			var statuses []github.Status
+			for context, state := range tc.contexts {
+				statuses = append(statuses, github.Status{


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)

@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?

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

spiffxp · 2021-08-03T19:21:55Z

prow/plugins/cla/cla_test.go

+		cc := plugins.CLAConfig{
+			CLAContextNames: []string{"cla/linuxfoundation", "EasyCLA"},
+		}
+		if err := handle(fc, logrus.WithField("plugin", pluginName), se, cc); err != nil {


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"

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?

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

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.

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.

spiffxp · 2021-08-03T19:27:07Z

prow/plugins/cla/cla_test.go

+			contexts: map[string]string{
+				"cla/linuxfoundation": "failure",
+				"EasyCLA":             "success",
+			},


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

sure. will do :)

anusha94 · 2021-08-12T17:26:37Z

/hold

MadhavJivrajani · 2021-09-17T09:03:07Z

FYI #23648
cc @spiffxp @anusha94

k8s-triage-robot · 2021-12-16T09:14:20Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

MadhavJivrajani · 2021-12-16T09:30:20Z

/remove-lifecycle stale

MadhavJivrajani · 2022-01-12T05:09:19Z

/close
closing this out in favor of a simpler switch over. Will send in a PR soon!

k8s-ci-robot · 2022-01-12T05:09:37Z

@MadhavJivrajani: Closed this PR.

In response to this:

/close
closing this out in favor of a simpler switch over. Will send in a PR soon!

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.

Add support for EasyCLA

856aeda

Added a new field `EasyCLAEnabled` in the plugins config and based on this field, if set will assign the claContextName to cla/easy-cla, else default to cla/linuxfoundation

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 30, 2021

k8s-ci-robot requested review from cjwagner and clarketm June 30, 2021 14:04

k8s-ci-robot added the area/prow Issues or PRs related to prow label Jun 30, 2021

k8s-ci-robot requested a review from nikhita June 30, 2021 14:12

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 1, 2021

MadhavJivrajani reviewed Jul 3, 2021

View reviewed changes

prow/plugins/cla/cla.go Outdated Show resolved Hide resolved

anusha94 added 2 commits July 9, 2021 20:14

Update handle and handleComment with parameter for easyCLAEnabled

064bec8

Fixed formatting errors

4fd9dbd

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 9, 2021

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 9, 2021

anusha94 changed the title ~~[WIP] Add support for EasyCLA~~ Add support for EasyCLA Jul 9, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2021

cjwagner reviewed Jul 9, 2021

View reviewed changes

Added a new CLAConfig struct and negative test cases

1125ef4

Add default values for CLAContextNames

d1d3fc2

anusha94 requested a review from cjwagner July 14, 2021 12:44

Adding default CLAConfig value in plugins_test

d05897a

k8s-ci-robot assigned jeefy Jul 20, 2021

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

spiffxp suggested changes Jul 20, 2021

View reviewed changes

k8s-ci-robot assigned spiffxp Jul 20, 2021

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 20, 2021

spiffxp reviewed Jul 20, 2021

View reviewed changes

prow/plugins/cla/cla.go Outdated Show resolved Hide resolved

spiffxp reviewed Jul 20, 2021

View reviewed changes

prow/plugins/cla/cla_test.go Show resolved Hide resolved

anusha94 added 3 commits July 24, 2021 20:44

Added documentation for CLAContextNames field

d998079

Modify TestCheckCLA to include multiple conflicting contexts

f427fba

Update formatting for cla_test.go

7b7bc3d

spiffxp suggested changes Aug 3, 2021

View reviewed changes

spiffxp reviewed Aug 3, 2021

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 12, 2021

mrbobbytables mentioned this pull request Sep 20, 2021

CLA Plugin: Add support for EasyCLA #22721

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 16, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 16, 2021

k8s-ci-robot closed this Jan 12, 2022

		// Valid status values are cla/linuxfoundation and EasyCLA
		if contains(cc.CLAContextNames, status.Context) {


		// No need to consider other contexts once you find the one you need.
		break

Add support for EasyCLA #22742

Add support for EasyCLA #22742

Conversation

anusha94 commented Jun 30, 2021 • edited by cjwagner

k8s-ci-robot commented Jun 30, 2021

k8s-ci-robot commented Jun 30, 2021

anusha94 commented Jun 30, 2021

anusha94 commented Jun 30, 2021

MadhavJivrajani commented Jul 1, 2021

MadhavJivrajani commented Jul 7, 2021 • edited

MadhavJivrajani commented Jul 9, 2021

cjwagner left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 14, 2021

anusha94 commented Jul 18, 2021

jeefy commented Jul 20, 2021

spiffxp 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

spiffxp Jul 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anusha94 commented Aug 2, 2021

spiffxp 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

anusha94 Aug 4, 2021 • edited

Choose a reason for hiding this comment

spiffxp Aug 4, 2021 • 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

anusha94 commented Aug 12, 2021

MadhavJivrajani commented Sep 17, 2021

k8s-triage-robot commented Dec 16, 2021

MadhavJivrajani commented Dec 16, 2021

MadhavJivrajani commented Jan 12, 2022

k8s-ci-robot commented Jan 12, 2022

anusha94 commented Jun 30, 2021 •

edited by cjwagner

MadhavJivrajani commented Jul 7, 2021 •

edited

spiffxp Jul 20, 2021 •

edited

anusha94 Aug 4, 2021 •

edited

spiffxp Aug 4, 2021 •

edited