-
Notifications
You must be signed in to change notification settings - Fork 1
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
[full-ci] Support ES7 #215
Conversation
rebased version of #185 |
9c4d658
to
7f2614c
Compare
@phil-davis same as with #185 - no idea why the acceptance tests are failing ..... can I ask you to take a look? THX |
7f2614c
to
8b255c5
Compare
The failed tests were reported in https://drone.owncloud.com/owncloud/search_elastic/1569/17/13
I applied the latest drone starlark code in PR #216 - that will make it report a comment properly to GitHub when a pipeline fails and the other pipelines are cancelled. I rebased this PR. Now I will review what is here, and see what CI thinks... |
💥 Acceptance tests pipeline apiLimitSearches-latest-mysql8.0-php7.2-es7.10 failed. The build has been cancelled. https://drone.owncloud.com/owncloud/search_elastic/1574/21/1 |
💥 Acceptance tests pipeline webUISE-master-firefox-mysql8.0-php7.2-es7.10 failed. The build has been cancelled. https://drone.owncloud.com/owncloud/search_elastic/1574/17/1 |
I cleaned up some text in README, just to have a new commit to push and get |
The new Elastic Search shows matches better. When searching for "ipsum" it finds it in The existing "old" Elastic Search code was showing the first line of the file, rather than the line containing the search string. I adjusted the test expectations. |
89d576d
to
5463d58
Compare
@phil-davis Could you please check why some tests are still failing? |
5463d58
to
2b11043
Compare
2b11043
to
45d5022
Compare
@phil-davis @DeepDiver1975 @pmaier1 Can we have a release this week please? |
I ended up reversing the test change in "Adjust indexOnlyMetadata.feature test" commit. I guess that something in "use owncloudops elastic image" made that test scenario go back to behaving the way that it originally behaved. Note: we still have issue #40 - the behavior has not changed for that issue. If this passes, then I will rebase-squash a bit to get rid of the unneeded commits. |
How is the test case failure related to the base image change? The CI was failing before already https://drone.owncloud.com/owncloud/search_elastic/1581 or what do I miss? |
Is #40 a release blocker? I guess not, as the issue is unrelated to the es7 compatibility update, or? |
I had actually modified that test scenario to get it passing with |
Not a blocker, it is an existing issue if someone wants to "wildcard" search filenames - search for "plan" and find a file called "business-plan.xls" or search for ".xls" and find all the Excel spreadsheets. That is functionality that "someone" can look into separately. |
eac7af1
to
434038e
Compare
I rebased-squashed so that there are 4 logical commits. This is good-to-go when CI finishes. |
@@ -7,6 +7,7 @@ | |||
<author>ownCloud GmbH</author> | |||
<dependencies> | |||
<owncloud min-version="10.3" max-version="10" /> |
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 to do about this? Do we have confidence that this will work with ownCloud 10.3 and PHP 7.2?
Or should we bump the owncloud min-version
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.
Question for @DeepDiver1975
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.
10.3 is no longer supported - right? just bump this to the latest and greatest .....
..... or chat with PM. 🤷
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.
@pmaier1 I guess we bump oC min_version to 10.7 ? Or 10.8? Or?
Unless there is a specific requirement to support this on some lower oC10.? and we then need to test against that.
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, usually we only bump those numbers if we know that an app version is not working with older core versions, e.g., due to some dependencies that are not fulfilled. The correct way would be to check this. But I don't see a real necessity to support it on lower oC 10 versions so I leave it up to you. Both ways are fine.
https://drone.owncloud.com/owncloud/search_elastic/1629/23/12
I haven't seen those failures before. And they look like "random" examples from scenario outlines. The same test pipeline against |
https://drone.owncloud.com/owncloud/search_elastic/1630/22/12
That is a completely different test scenario in a different test suite. It seems that we have intermittent differences. |
Kudos, SonarCloud Quality Gate passed! |
The last run of CI passed. IMO when we have clarified the support (or not) for older ES5 and ES6 then we can either merge this, or adjust it to support multiple major ES versions. |
No description provided.