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.

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 checklist item for the CLA #4056

Closed
wants to merge 1 commit into from
Closed

Add checklist item for the CLA #4056

wants to merge 1 commit into from

Conversation

mdavis332
Copy link
Contributor

@mdavis332 mdavis332 commented Aug 23, 2022

What does this PR do?

Adjusts PR template to add the requirement of having signed the CLA.

Checklist

@mdavis332 mdavis332 requested a review from a team as a code owner August 23, 2022 15:21
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Hey Michael, the check is guaranteed by the Jenkins job. Do you see strong reasons why we need to mention it also in the template? It isn't something we do.

@elasticmachine
Copy link

elasticmachine commented Aug 23, 2022

❕ Build Aborted

The PR is not allowed to run in the CI yet

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Start Time: 2022-09-12T08:53:04.994+0000

  • Duration: 3 min 47 sec

Steps errors 3

Expand to view the steps failures

Load a resource file from a library
  • Took 0 min 0 sec . View more details here
  • Description: approval-list/elastic/integrations.yml
Google Storage Download
  • Took 0 min 0 sec . View more details here
Error signal
  • Took 0 min 0 sec . View more details here
  • Description: githubApiCall: The REST API call https://api.github.com/orgs/elastic/members/mdavis332 return the message : java.lang.Exception: httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/mdavis332 : httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/mdavis332 : Code: 404Error: {"message":"User does not exist or is not a member of the organization","documentation_url":"https://docs.github.com/rest/reference/orgs#check-organization-membership-for-a-user"}

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@mdavis332
Copy link
Contributor Author

Hey Michael, the check is guaranteed by the Jenkins job. Do you see strong reasons why we need to mention it also in the template? It isn't something we do.

Hi Marcin,

As a recent, first-time contributor to this repo, I had tried to follow all the guidance and steps I could find documented anywhere (everywhere? :) to ensure my initial submission was as error-free as possible.

One thing that came up though was the failed CI step due to my having not completed the CLA at the time. There has since been a merge to add this into the repo's docs.

Another thought I had today is that if that requirement had been in the PR template, I would have seen it and known to complete it prior to opening a PR.

That's my thought process, but I defer to your team on whether the PR template is the right place for such an item.

@botelastic
Copy link

botelastic bot commented Oct 12, 2022

Hi! We just realized that we haven't looked into this PR in a while. We're sorry! We're labeling this issue as Stale to make it hit our filters and make sure we get back to it as soon as possible. In the meantime, it'd be extremely helpful if you could take a look at it as well and confirm its relevance. A simple comment with a nice emoji will be enough :+1. Thank you for your contribution!

@botelastic botelastic bot added the Stalled label Oct 12, 2022
@jsoriano
Copy link
Member

jsoriano commented Nov 8, 2022

Hey @mdavis332,

I think that the text you added in the developer guidelines (thanks!) together with the CI check is fine enough. Adding an additional check to the PR checklist would require any developer to have to check it every time they open a PR, while the check is going to be done by CI in any case. I see how this can help to first-time contributors, but this adds an small additional thing to do in any pull request also for frequent contributors.
We have followed this approach of checking the CLA only with CI in other projects and it has worked fine so far.

I am going to close this by now, happy to reopen it if more people have problems with this.

Thanks!

@jsoriano jsoriano closed this Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants