Skip to content

Conversation

@rneatherway
Copy link
Contributor

Hi! I was browsing the project on lgtm.com and came across a couple of alerts where different types are being compared using .equals(). In these cases the fix seemed pretty obvious so I've gone ahead and done it here.

I've already gone ahead and signed the CLA. I hope these fixes are useful!

Full disclosure -- I am one of the developers working on lgtm.com

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

@rneatherway thanks for reporting this, both cases look like they should be fixed too me. Also /cc @nknize and @jdconrad to make sure they agree. Those cases should probably also get some related unit tests.
Will kick of a test run in a minute and run the CLA checker once again to see why it still fails.

@cbuescher
Copy link
Member

@elasticmachine test this please

@cbuescher
Copy link
Member

@rneatherway your CLA signature is in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@rneatherway
Copy link
Contributor Author

I already have both the one I signed with and the one I committed with associated with my Github profile, is there anything else I need to do?

@cbuescher
Copy link
Member

Thanks, normally no. Maybe the internal checker just needs to catch up.

@nknize nknize self-requested a review December 6, 2017 18:52
Copy link
Contributor

@nknize nknize left a comment

Choose a reason for hiding this comment

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

LGTM

@rneatherway
Copy link
Contributor Author

Is there any way to refresh the CLA check? If it is easier I can just resign it with the email address I used to commit here.

@cbuescher
Copy link
Member

@rneatherway I tried retriggering, usually it works, trying to find out whats wrong and waiting for feedback atm.

@karmi
Copy link
Contributor

karmi commented Dec 7, 2017

Hi @rneatherway, I maintain the CLA checker service on our side, and wanted to ask if you can double check that the e-mail from the Git commit is indeed added to your Github profile. Normally, as @cbuescher says, that's enough for the CLA check to be green, but is seems to fail in this case...

@rneatherway
Copy link
Contributor Author

Yes I have, it is even my primary email address. I think if it wasn't on my profile then Github wouldn't have known that the commit was by rneatherway.

@cbuescher cbuescher self-assigned this Dec 7, 2017
@karmi
Copy link
Contributor

karmi commented Dec 7, 2017

@rneatherway, I agree, that's how it works. However, I don't have any good explanation why this PR isn't then green-marked, as it normally happens... Would be so kind and help us to debug this by trying to a) signing the CLA with the same e-mail as in Git, b) changing the Git email to the one in signature?

@rneatherway
Copy link
Contributor Author

Sure, I've done 'a'

@cbuescher cbuescher added :Analytics/Geo Indexing, search aggregations of geo points and shapes :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >bug v6.2.0 v7.0.0 labels Dec 7, 2017
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

@rneatherway thanks, looks all green now, I will merge this in.

@cbuescher cbuescher merged commit 057efea into elastic:master Dec 7, 2017
@karmi
Copy link
Contributor

karmi commented Dec 7, 2017

@rneatherway, many thanks!, I still don't know why this happened, but it's good to know there's a bit of a "brute force" solution like this one.

@rneatherway rneatherway deleted the definition-arguments-check branch December 7, 2017 14:12
@rneatherway
Copy link
Contributor Author

Great! I'll see if I can fix anything else.

@jpountz jpountz removed the :Analytics/Geo Indexing, search aggregations of geo points and shapes label Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>bug :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v6.1.0 v6.2.0 v7.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants