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

[full-ci] update reva to v1.9.1 #2280

Merged
merged 3 commits into from
Jul 13, 2021
Merged

[full-ci] update reva to v1.9.1 #2280

merged 3 commits into from
Jul 13, 2021

Conversation

micbar
Copy link
Contributor

@micbar micbar commented Jul 11, 2021

Preparation for the ocis release.

To do

  • Wait for full ci run
  • Update expected failures

@micbar micbar requested review from C0rby, fschade and refs July 11, 2021 15:56
@micbar micbar force-pushed the update-reva-1.9.1 branch 3 times, most recently from ef3817a to a2cdb72 Compare July 12, 2021 06:46
@micbar micbar force-pushed the update-reva-1.9.1 branch 2 times, most recently from f8b258b to db09f9c Compare July 12, 2021 07:53
@micbar micbar requested a review from phil-davis July 12, 2021 07:59
@micbar micbar force-pushed the update-reva-1.9.1 branch from db09f9c to b79eba9 Compare July 12, 2021 08:48
@phil-davis
Copy link
Contributor

@micbar sorry! We updated the web commit id in another PR and there were expected-failures changes that cause conflicts here.

Do you want me to rebase and sort it out?

@micbar micbar force-pushed the update-reva-1.9.1 branch from b79eba9 to f5fc95e Compare July 12, 2021 08:58
@micbar
Copy link
Contributor Author

micbar commented Jul 12, 2021

pushed again after rebase

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

LGTM

@phil-davis
Copy link
Contributor

https://drone.owncloud.com/owncloud/ocis/5974/52/6
Unexpected failure or crash of the nightwatch test run

That caused a few "unexpected passed scenarios" - because actually that reports the scenarios that were expected to fail but did not fail. If the test run crashed early, then there might be scenarios that did not fail because they did not run at all.

I will restart CI...

@phil-davis
Copy link
Contributor

https://drone.owncloud.com/owncloud/ocis/5976/52/6
Same crash in the test run. I will get someone to take a look. I guess that something in the "new reva" is making the server behave different, the web UI then responds differently, and some expected UI element is not there at all, or????

@phil-davis
Copy link
Contributor

@dpakach is taking a look. We might have to adjust the test code in web to better handle unusual trashbin responses.

@dpakach
Copy link
Contributor

dpakach commented Jul 12, 2021

while the tests crashing in the middle of the run is an issue itself, the real problem here is because we cannot send dav request to trashbin with Depth header now

curl -XPROPFIND https://localhost:9200/remote.php/dav/trash-bin/admin -H Depth:2 -k -u admin:admin -v

this gives http 400 with no body

@phil-davis
Copy link
Contributor

@micbar "we cannot send dav request to trashbin with Depth header now"

That is a bit surprising - a "new reva" should improve things. It is strange that something like this would regress.

@phil-davis
Copy link
Contributor

Fix to make the test code handle the empty body possibility, and fail the test gracefully: owncloud/web#5511
After that is merged, we can bump the web commit id, and then CI should run with just "a few" new failures.

Tomorrow we will sort out core API tests to cover this PROPFIND-of-a-whole-trashbin-folder scenario - this should have been found in core API tests against cs3org/reva first.

@phil-davis
Copy link
Contributor

owncloud/web#5511 has been merged. OCIS PR #2287 is updating the web commit id (that was already happening anyway, so I added an extra commit there).

After that is merged, we can rebase here and we should get a "more sane" CI result that has only the real failed scenarios.

@phil-davis
Copy link
Contributor

phil-davis commented Jul 12, 2021

https://drone.owncloud.com/owncloud/ocis/5990/52/6

@micbar the webUIWebdavLocks* and webUIUpload and webUITrashbinRestore/trashbinRestore.feature:138 scenarios need to go back in expected-failures.

and some like webUITrashbinRestore/trashbinRestore.feature:260 need to be added to expected-failures. They fail now due to same problem with PROPFIND that @dpakach mentioned #2280 (comment)

@phil-davis phil-davis force-pushed the update-reva-1.9.1 branch 2 times, most recently from f036bda to e0d6fe1 Compare July 13, 2021 02:58
@phil-davis
Copy link
Contributor

I put back the expected-failures for web and rebased. CI should pass now.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@butonic
Copy link
Member

butonic commented Jul 13, 2021

@micbar "we cannot send dav request to trashbin with Depth header now"

That is a bit surprising - a "new reva" should improve things. It is strange that something like this would regress.

As part of cs3org/reva#1827 reva no longer 'just ignores' the depth header on trash requests: https://github.com/cs3org/reva/pull/1827/files#diff-2219ddd689ea2f1f39fd79ba85e4ba622a3a00cb612bc7164de2a46864d87528R149-R162

TBH why do we need to be able to support Depth: 2 ? Not present, 0, 1 or infinity are the cases that matter. or do we have clients making requests with other depths?

@phil-davis
Copy link
Contributor

TBH why do we need to be able to support Depth: 2 ? Not present, 0, 1 or infinity are the cases that matter. or do we have clients making requests with other depths?

@dpakach is adding some core API test scenarios - see issue owncloud/core#38981 - and he will find out which Depth currently work in oC10, reva and OCIS.

We could discuss there about what Depth should be properly supported by an implementation, and which are not required. And then make the appropriate set of test cases.

@wkloucek wkloucek merged commit cf6f71d into master Jul 13, 2021
@delete-merged-branch delete-merged-branch bot deleted the update-reva-1.9.1 branch July 13, 2021 07:03
ownclouders pushed a commit that referenced this pull request Jul 13, 2021
Merge: ddd261f 2433cab
Author: Willy Kloucek <[email protected]>
Date:   Tue Jul 13 09:03:31 2021 +0200

    Merge pull request #2280 from owncloud/update-reva-1.9.1

    [full-ci] update reva to v1.9.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants