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

Share type sciencemesh #36228

Closed

Conversation

michielbdejong
Copy link
Contributor

Summary

Add the sciencemesh share type in all the places where share types are enumerated.

@solracsf solracsf added this to the Nextcloud 26 milestone Jan 18, 2023
@solracsf solracsf added 3. to review Waiting for reviews 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jan 18, 2023
@solracsf
Copy link
Member

Can you look at PSALM + failed tests?

*/
private function getSciencemeshShareHelper() {
if (!$this->appManager->isEnabledForUser('sciencemesh')) {
throw new QueryException();

Check notice

Code scanning / Psalm

DeprecatedClass

OCP\AppFramework\QueryException is marked deprecated
@skjnldsv skjnldsv removed their request for review January 24, 2023 13:54
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Looks reasonable, just two minor comments inline :)

@michielbdejong
Copy link
Contributor Author

I'll fix apps/files_sharing/tests/MountProviderTest.php and apps/files_sharing/tests/Controller/ShareAPIControllerTest.php

@michielbdejong
Copy link
Contributor Author

michielbdejong commented Jan 30, 2023

Hm, only one 'spreed' integration test is failing now https://drone.nextcloud.com/nextcloud/server/29215/45/4
It's missing a file from autoload.php
I'll try to run this test in a dev environment.

server/.drone.yml

Lines 1230 to 1264 in aee6b37

kind: pipeline
name: integration-sharing-v1-video-verification
steps:
- name: submodules
image: ghcr.io/nextcloud/continuous-integration-alpine-git:latest
commands:
- git submodule update --init
- name: install-talk
image: ghcr.io/nextcloud/continuous-integration-php8.0:latest
commands:
# JavaScript files are not used in integration tests so it is not needed to
# build them.
- git clone --depth 1 https://github.com/nextcloud/spreed apps/spreed
- cd apps/spreed
- composer --version
- composer self-update --2
- composer install --no-dev
- cd ../..
- name: integration-sharing-v1-video-verification
image: ghcr.io/nextcloud/continuous-integration-integration-php8.0:latest
commands:
- bash tests/drone-run-integration-tests.sh || exit 0
- ./occ maintenance:install --admin-pass=admin --data-dir=/dev/shm/nc_int
- cd build/integration
- ./run.sh sharing_features/sharing-v1-video-verification.feature
trigger:
branch:
- master
- stable*
event:
- pull_request
- push

@michielbdejong
Copy link
Contributor Author

When I run this in a dev environment I see:

32 scenarios (32 passed)
386 steps (386 passed)

So based on that I think the test failure might have been intermittent? Can someone maybe rerun the integration-sharing-v1-video-verification test on drone?

@michielbdejong
Copy link
Contributor Author

@juliushaertl can you have a look please when you have time?

@blizzz blizzz mentioned this pull request Feb 1, 2023
*/
private function getSciencemeshShareHelper() {
if (!$this->appManager->isEnabledForUser('sciencemesh')) {
throw new QueryException();

Check notice

Code scanning / Psalm

DeprecatedClass

OCP\AppFramework\QueryException is marked deprecated
@michielbdejong
Copy link
Contributor Author

@juliushaertl please let us know if there is anything else we can do, we are waiting for this to be merged asap if possible.

@michielbdejong michielbdejong requested review from nickvergessen and removed request for come-nc and artonge February 11, 2023 08:06
* @deprecated 21.0.0 - use IShare::TYPE_SCIENCEMESH instead
*/
public const SHARE_TYPE_SCIENCEMESH = 15;
// Note to developers: Do not add new share types here
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Note to developers: Do not add new share types here
// Note to developers: Do not add new share types here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I think this is the change I committed in ddd991d - it should be 1 tab, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The GitHub viewer here has 8-space tabs and the one for ddd991d displays 4-space tabs. Also, when I click 'Sign off and commit suggestion' here, nothing happens, so I think this is correct OK now.
Will double-check this after the rebase.

@juliusknorr
Copy link
Member

Triggered the checks again. Can you maybe rebase to latest master as well when fixing the indentation issue? The drone failure looks unrelated.

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Code looks good 👍

Mind to do another rebase to latest master to retriever the failed tests? Looks unrelated to me though...

Signed-off-by: Michiel de Jong <michiel@unhosted.org>
@michielbdejong michielbdejong requested review from juliusknorr and removed request for nickvergessen February 20, 2023 10:29
@michielbdejong
Copy link
Contributor Author

This workflow requires approval from a maintainer.

@michielbdejong
Copy link
Contributor Author

@juliushaertl can you trigger CI please?

@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@michielbdejong
Copy link
Contributor Author

@skjnldsv can you maybe approve the workflows so this can be merged?

@michielbdejong
Copy link
Contributor Author

This has been stuck for 11 days now...

@michielbdejong
Copy link
Contributor Author

@juliushaertl Please merge this today if possible.

@michielbdejong
Copy link
Contributor Author

I'm giving a presentation about this at 3pm.
I think @karlitschek said we are maybe already too late for release 26 and we should target this to release 27 now?
What is the next step here? It would be great if this can still be merged today, before my presentation!

@AndyScherzinger
Copy link
Member

@artonge can you look into the updating of the js bundle please? Thanks in advance

@artonge artonge mentioned this pull request Mar 8, 2023
@artonge
Copy link
Contributor

artonge commented Mar 8, 2023

Can't push on not owned origin. I created a mirror here: #37139

@juliusknorr
Copy link
Member

Closing as #37139 has been merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for new 'sciencemesh' share type
6 participants