Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Add support for EasyCLA #22742
Add support for EasyCLA #22742
Changes from 8 commits
856aeda
064bec8
4fd9dbd
1125ef4
9951f53
13e6fe9
d1d3fc2
d05897a
d998079
f427fba
7b7bc3d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
combined.Statuses
sorted at this point? or could we be walking these in random order?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spiffxp Can you please elaborate on competing statuses? Going by the existing code
if status.Context == claContextName
, I assumed thefor loop
should not continue if it is not one ofcla/linuxfoundation
orEasyCLA
Or are you saying this entire
for loop
should be rewritten such that applying ofClaNo
orClaYes
should be outside the for loop?Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrbobbytables Do you know about the sort order of
combined.Statuses
? Or rather, how can I check / verify the order?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the
break
statement so that all contexts will be processed.There was a problem hiding this comment.
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?There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
and vice-versa
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the docs,
The most recent status for each context is returned
.I think my question is - are
EasyCLA
andcla/linuxfoundation
considered as two different contexts or different names for a singleCLA
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I'm assuming I'm going to see two entries, one called
cla/linuxfoundation
and one calledEasyCLA
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 whenevercla/linuxfoundation
is update, and again wheneverEasyCLA
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.
There was a problem hiding this comment.
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:
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.