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

Nextcloud backup: allow subdir #4077

Merged
merged 5 commits into from Jan 2, 2021

Conversation

maxxer
Copy link
Contributor

@maxxer maxxer commented Apr 30, 2020

Fixes #3988

@AdSchellevis
Copy link
Member

@fabianfrz do you have time to spin this for a test?

@fabianfrz
Copy link
Member

@AdSchellevis crashes immediately:

Fatal error: Uncaught Error: Call to undefined function OPNsense\Backup\basedir() in /usr/local/opnsense/mvc/app/library/OPNsense/Backup/Nextcloud.php:147 Stack trace: #0 /usr/local/www/diag_backup.php(286): OPNsense\Backup\Nextcloud->backup() #1 {main} thrown in /usr/local/opnsense/mvc/app/library/OPNsense/Backup/Nextcloud.php on line 147

@fabianfrz
Copy link
Member

log after fixing the issue by replacing it by basename(), opnsense/backup as file name:

config[60397]: Error while fetching filelist from Nextcloud

{
  "url":"https:\/\/nextcloud.test\/remote.php\/dav\/files\/opnsensebkp\/Backup",
  "content_type":"text\/html; charset=UTF-8",
  "http_code":404,
  "header_size":1220,
  "request_size":205,
  "filetime":-1,
  "ssl_verify_result":0,
  ...
}

Tested on OPNsense 20.1.r_12 (amd64/LibreSSL) - I don't think somebody changed the nextcloud code inbetween

Copy link
Member

@fabianfrz fabianfrz left a comment

Choose a reason for hiding this comment

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

code is broken and not ready to deploy. Test failed.

@maxxer
Copy link
Contributor Author

maxxer commented May 4, 2020

Hi @fabianfrz thanks for the feedback.

About the last issue, one question: does opnsensebkp directory exist on your Nextcloud? Because indeed the plugin doesn't create all the path to the backup dir, I left the original behaviour that will create just the last one (Backup in your example). Is it expected differently?

@maxxer
Copy link
Contributor Author

maxxer commented May 4, 2020

I'm sorry for the misspelled function name, I meant to use dirname but accidentally copied the wrong file to git :(

@fabianfrz
Copy link
Member

now the php error is gone but the backup still fails.

@maxxer
Copy link
Contributor Author

maxxer commented May 5, 2020

Does the parent directory exist?

@fabianfrz
Copy link
Member

nope, but that is what the code is expected to do, create the full directory hierarchy if missing.

@maxxer
Copy link
Contributor Author

maxxer commented May 5, 2020

Ok, I will implement that. I asked two comments ago but got not answer...

@fabianfrz
Copy link
Member

Sorry I forgot the answer but if you read the source, there is already MKCOL implemented which is doing this so yes, it is the current behaviour.

@maxxer
Copy link
Contributor Author

maxxer commented May 23, 2020

I've updated the patch and merge against current master, I hope it's fine now

@maxxer
Copy link
Contributor Author

maxxer commented Sep 4, 2020

Any chance to get another review?

@hasechris
Copy link

Hii,

Any chance to get another review?
@fabianfrz could you please review this merge?

Thanks in advance

Copy link
Member

@fabianfrz fabianfrz left a comment

Choose a reason for hiding this comment

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

I found some improvements for the models but if this changes are in the PR, I can give it a test run.

@fabianfrz
Copy link
Member

fabianfrz commented Jan 2, 2021

Tested now successfully:

Screenshot_20210102_103815
Screenshot_20210102_103916
Screenshot_20210102_103959

@AdSchellevis you can merge it now if you like.

@AdSchellevis
Copy link
Member

@maxxer @fabianfrz thanks! I will merge it to master

@AdSchellevis AdSchellevis merged commit 074ccb9 into opnsense:master Jan 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Backup to Nextcloud
4 participants