Skip to content

Try to delete the cypress folder of the viewer app#16288

Merged
rullzer merged 1 commit into
masterfrom
fix/16229/cleanup-cypress-folder
Jul 8, 2019
Merged

Try to delete the cypress folder of the viewer app#16288
rullzer merged 1 commit into
masterfrom
fix/16229/cleanup-cypress-folder

Conversation

@MorrisJobke

Copy link
Copy Markdown
Member

Fixes #16229

@MorrisJobke

Copy link
Copy Markdown
Member Author

As proposed by @jospoortvliet

@MorrisJobke

Copy link
Copy Markdown
Member Author

/backport to stable16

@MorrisJobke

Copy link
Copy Markdown
Member Author

I tested this on Nextcloud 16.0.2 and ran the occ maintenance:repair manually (which is triggered during the upgrade as well) and it worked.

/** @var Checker $checker */
private $checker;

private $pathToViewerApp = __DIR__ . '/../../../../apps/viewer';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can't we get it from the app manager? in case it was moved?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I do a file_exists anyways and it should only matter if the app was installed via the release package and not updated via appstore. So this is just the easiest way without the problem of dealing with multiple app directories.

@kesselb

kesselb commented Jul 8, 2019

Copy link
Copy Markdown
Contributor

I would prefer RemoveCypressFiles

}

public function run(IOutput $output): void {
$file = $this->pathToViewerApp . '/cypress.json';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

private function shouldRun(): bool {
return version_compare(
$this->config->getSystemValue('version', '0.0.0.0'),
'16.0.0.0',
'<='
);
}
public function run(IOutput $output): void {
if ($this->shouldRun()) {
$this->repair($output);
}
}

I guess you want to release it today? You could add something like this if there is some time left.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It was there and the check for the actual outcome is far better IMO than the "lets assume some version number and check for that". And here the check for the outcome was pretty straight forward so I reduced the code to be better readable by just put it where it is checked.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm okay with that. Would it make sense to mark this as deprecated to remove it in the future?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Let me add this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In general that was also the idea of putting those repair steps in the NC16 folder for example. Because you know when they were added.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I also did some cleanup in #16292

@juliusknorr

Copy link
Copy Markdown
Member

I think this should actually not be required. From a look at the updater code it seems that it only deletes files from apps listed in shipped.json.
https://github.com/nextcloud/updater/blob/master/lib/Updater.php#L787-L796

For viewer that was not the case in 16.0.1 which is why the update to 16.0.2 doesn't delete those files. (See https://github.com/nextcloud/server/pull/15592/files)

This means that if you have installed the RC of 16.0.2 and update to 16.0.2 the error will be gone, which might explain that some of us could not reproduce the issue with the release.

But I'm also fine to have this in, just to be sure 😉

@juliusknorr

Copy link
Copy Markdown
Member

Makes of course still sense to have this cleanup job, in case someone goes from 16.0.1 to 16.0.3 then 👍

@MorrisJobke

Copy link
Copy Markdown
Member Author

For viewer that was not the case in 16.0.1 which is why the update to 16.0.2 doesn't delete those files. (See https://github.com/nextcloud/server/pull/15592/files)

This means that if you have installed the RC of 16.0.2 and update to 16.0.2 the error will be gone, which might explain that some of us could not reproduce the issue with the release.

The argument by @jospoortvliet was that we should try to fix this proactive instead of assuming that it will resolve in the future by itself. ;) And as this is straight forward I'm fine with the proactive repair step.

@MorrisJobke
MorrisJobke force-pushed the fix/16229/cleanup-cypress-folder branch from ddd71d5 to d1e4e7d Compare July 8, 2019 12:28
@MorrisJobke

Copy link
Copy Markdown
Member Author

I would prefer RemoveCypressFiles

Done ✅

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@jospoortvliet

Copy link
Copy Markdown
Member

thanks, good work 👍

@rullzer
rullzer merged commit b648ae6 into master Jul 8, 2019
@delete-merged-branch
delete-merged-branch Bot deleted the fix/16229/cleanup-cypress-folder branch July 8, 2019 17:18
@backportbot-nextcloud

Copy link
Copy Markdown

backport to stable16 in #16297

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews bug high

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failed Code Integrity because of EXTRA_FILE after Update from 16.0.1 to 16.0.2

7 participants