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

[tests-only] [full-ci] Remove fsweb.test.owncloud.com SMB PHP unit test pipeline #40426

Merged
merged 1 commit into from
Jan 4, 2023

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Oct 14, 2022

Description

fsweb.test.owncloud.com is a "legacy" Windows SMB server that is now only used for this PHP unit test pipeline. It is old and difficult to support.

The SMB settings for using it in the test pipeline are stored in https://github.com/owncloud/core/blob/master/tests/drone/configs/config.files_external.smb-windows.php

https://github.com/owncloud/core/blob/master/tests/drone/test-phpunit.sh#L30 has the code that sets up the pipeline for running the test.

https://github.com/owncloud/core/blob/master/apps/files_external/tests/Storage/SmbTest.php is run.

Note: those tests are also run in a pipeline against a Samba server that runs in docker, so they are run against an SMB server (which is not a "real" Windows server) The questions is, do we want to run against a real Windows SMB server, and if so, what version of Windows server(s), and how will we provide them to CI.

This PR removes the test pipeline that runs against fsweb.test.owncloud.com

How Has This Been Tested?

CI

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@phil-davis phil-davis self-assigned this Oct 14, 2022
@xoxys xoxys self-requested a review October 14, 2022 09:59
@owncloud owncloud deleted a comment from update-docs bot Oct 14, 2022
@phil-davis phil-davis changed the title Remove fsweb.test.owncloud.com SMB PHP unit test pipeline [tests-only] [full-ci] Remove fsweb.test.owncloud.com SMB PHP unit test pipeline Oct 14, 2022
@phil-davis phil-davis requested a review from jnweiger October 14, 2022 10:01
@phil-davis phil-davis force-pushed the remove-fsweb-smb-test-pipeline branch from d4498cf to c0bb0a9 Compare October 14, 2022 10:02
@phil-davis phil-davis requested a review from dragotin October 14, 2022 10:03
@xoxys
Copy link
Contributor

xoxys commented Oct 14, 2022

@phil-davis
Copy link
Contributor Author

@jnweiger @dragotin please sort out if we can drop this test pipeline. And what action to take next (do nothing more, provide another Windows SMB server somehow and reinstate the pipeline in the future, or..)

@xoxys
Copy link
Contributor

xoxys commented Oct 14, 2022

Do nothing is no option. If the pipeline has to stay, a new Window server setup is a hard requirement. The current setup has to go in any case.

@dragotin
Copy link
Contributor

This really seems to be a difficult case. I understand that @phil-davis said that we continue to run the tests against a Samba server even if we drop this real-windows system. If I got that right, I am for merging this PR and drop the machine and do nothing more. It seems nobody cares anyway.

Note that I do not know the code nor the usecase or who has written this code, I am just deciding because it seems I can not hide out because of my role. And I do not want to, lets get rid of some stuff now, and look into setting up a new test later if it turns out that the Samba based tests are fooling us.

So lets give @hodyroff @phil-davis @DeepDiver1975 @jvillafanez @mbarz @jnweiger a last couple of days (until Wednesday, 2022-10-19) for a veto, and if not, please

  1. merge this PR (@phil-davis)
  2. Kill the real windows server (@xoxys )
  3. and be happy with running the tests only against Samba (all)

@jvillafanez
Copy link
Member

I think we'll have to test against a windows machine some way or another. The behavior should (but might not) be the same with samba and with windows, but in any case, we have to ensure it and detect as well as fix any particular difference that there might be.

There are a list of features that we have to check:

  • DFS
  • ACL support (we need to take care of primary groups in windows)
  • AD groups
  • Kerberos authentication

I remember there have been some issues in the past about DFS with replication in a windows machine, due to some special folders that windows creates for the replication. I don't think this is supported with samba. There might be additional details that aren't implemented in one place, or that behaves differently.

From my point of view, the connection against a windows machine should be the priority, being samba the second option.

About the pipeline itself, I don't know how deep the tests are, so maybe we can remove it assuming there is no difference in what is being tested (basic connectivity checks should work the same way). However, we have to have a windows environment even if it's just for manual feature verification.
I don't want a "it works on my machine" and nobody else is able to reproduce neither the issue nor the fix.

@DeepDiver1975
Copy link
Member

We originally had the test running with Samba server as well as Windows server because both behave slightly different in terms of response codes of the smb api calls.
Initially we had an implementation which worked perfect with Samba server but did not work at all with a Windows server due to explained reason above.

As long as we support Samba AND Windows server we better run the automated integration tests with these two.
(which is anyhow a minimalist setup - in an ideal world we would test with different versions of these two products.....)

@DeepDiver1975
Copy link
Member

I don't want a "it works on my machine" and nobody else is able to reproduce neither the issue nor the fix.

💋

Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

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

Killing it is not an option

@xoxys
Copy link
Contributor

xoxys commented Oct 17, 2022

Then we are back at the beginning. Somebody need to be named who will set up and maintain a proper Windows Server setup, the current one has to go.

@dragotin
Copy link
Contributor

Yes, looks like @DeepDiver1975 and @jvillafanez volunteered. Yes, I know, you haven't. But we need somebody, and you are the ones who are interested and (for good reasons) insist on having it.

@DeepDiver1975
Copy link
Member

Yes, looks like @DeepDiver1975 and @jvillafanez volunteered.

Then I don't care - kill it 🤷

@jvillafanez
Copy link
Member

I could manage to setup something, but I don't think I'm the guy to secure and administer a more or less public windows instance. We might also need something a bit more professional, setting up the instance with ansible or a similar tool, so we could replicate the instance if needed, or at least a big part of it.

@phil-davis
Copy link
Contributor Author

Also see issue #40457 that was raised today - the fsweb server seems to be down again.

@hodyroff
Copy link

@GeraldLeikam volunteered for the Windows Server.
@jnweiger

@crrodriguez
Copy link

I kept it alive. but I strongly suggest having this machoine around. just provision a windows smb server at azure instead . that will probably be much better.

@phil-davis phil-davis force-pushed the remove-fsweb-smb-test-pipeline branch from c0bb0a9 to 9158e23 Compare November 15, 2022 08:44
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    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

@DeepDiver1975
Copy link
Member

I am sick of this situation -> merge

@phil-davis
Copy link
Contributor Author

PR #40570 gets this test pipeline working again.

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.

8 participants