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 parent aggregation #1616

Merged
merged 3 commits into from Apr 1, 2019
Merged

Add parent aggregation #1616

merged 3 commits into from Apr 1, 2019

Conversation

@SosthenG
Copy link
Contributor

@SosthenG SosthenG commented Mar 6, 2019

I also updated the docker elasticsearch version to 6.6.1 to be able to launch the test.

Copy link
Owner

@ruflin ruflin left a comment

Overall LGTM. I have one ask for the ES version change.

@@ -1,5 +1,5 @@
#https://www.docker.elastic.co/
FROM docker.elastic.co/elasticsearch/elasticsearch:6.5.2
FROM docker.elastic.co/elasticsearch/elasticsearch:6.6.1
Copy link
Owner

@ruflin ruflin Mar 7, 2019

Choose a reason for hiding this comment

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

Could you open a separate PR with this change to make sure it does not have any other affects? With this change, we normally also update the README and all the places where we mention what we test with (just search for 6.5.2)

Copy link
Contributor Author

@SosthenG SosthenG Mar 25, 2019

Choose a reason for hiding this comment

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

Done in #1620

Copy link
Owner

@ruflin ruflin Mar 27, 2019

Choose a reason for hiding this comment

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

Thanks, as soon as the other one is in, could you rebase on top of it?

Copy link
Contributor Author

@SosthenG SosthenG Mar 27, 2019

Choose a reason for hiding this comment

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

Sure, I will do it!

Copy link
Owner

@ruflin ruflin left a comment

Not sure if you tried to rebase or merge but seems like something went wrong :-)

### Improvements
* Added `native_function_invocation` CS rule [#1606](https://github.com/ruflin/Elastica/pull/1606)
* Elasticsearch test version changed from 6.5.2 to 6.6.1 [#1620](https://github.com/ruflin/Elastica/pull/1620)
Copy link
Owner

@ruflin ruflin Mar 27, 2019

Choose a reason for hiding this comment

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

Seems like a bit more changes in the changelog then there should be?

Copy link
Contributor Author

@SosthenG SosthenG Mar 27, 2019

Choose a reason for hiding this comment

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

Sorry this is my first PR, I'm still discovering how to use git in such way...
I did a rebase of my parent-agg branch onto your origin/master branch but I don't really understand what happened after (my IDE added an empty merge, I don't know why, but I definitely did a rebase). When I try to rebase again, it says that my branch is up-to-date with your master, and for me it seems that way, I suppose I don't understand well what this rebase should have done.
Can you explain me what's wrong and how to fix it please? Really sorry for the inconvenience.

Copy link
Owner

@ruflin ruflin Mar 28, 2019

Choose a reason for hiding this comment

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

One thing that potentially might have happened, is that your master is not up-to-date. Can you go to your master and pull the most recent version? You can use git log to see which commit your master is on. Today it should be d0433d6 (the id or the prefix of it you should see in your git log output on the top).

Copy link
Contributor Author

@SosthenG SosthenG Mar 28, 2019

Choose a reason for hiding this comment

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

Actually, the first time I tried to rebase directly on your master, not mine (which was not sync with yours).
I tried to redo the operation with my master, after setting it in sync with yours.
It looks good to me this time, I hope it is :-)

README.md Outdated
@@ -17,7 +17,7 @@ All library issues should go to the [issue tracker from github](https://github.c

Compatibility
-------------
This release is compatible with all Elasticsearch 6.x releases. It was tested with version 6.5.2.
This release is compatible with all Elasticsearch 6.x releases. It was tested with version 6.6.1.
Copy link
Owner

@ruflin ruflin Mar 27, 2019

Choose a reason for hiding this comment

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

That should go away with a rebase? Also below?

@ruflin
Copy link
Owner

@ruflin ruflin commented Mar 29, 2019

PR looks good, but there is a failing test. Unfortunately can't really see how it's related to this PR especially as only 1 PHP version failed: https://travis-ci.org/ruflin/Elastica/jobs/512448687

@ruflin
Copy link
Owner

@ruflin ruflin commented Apr 1, 2019

I restarted the test to see if it's just flaky.

@ruflin ruflin merged commit c7cfa1c into ruflin:master Apr 1, 2019
1 check passed
@ruflin
Copy link
Owner

@ruflin ruflin commented Apr 1, 2019

@SosthenG Thanks for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants