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

Make owncloudcmd return code depend on sync result #3936

Closed
nickvergessen opened this Issue Oct 9, 2015 · 8 comments

Comments

Projects
None yet
7 participants
@nickvergessen
Contributor

nickvergessen commented Oct 9, 2015

Smashbox currently has code (added from cern) that ignores the returncode on the sync.
After speaking with @guruz it should be checked, so the return code is reliable and we can notify the user when the returncode is not-successful.

@moscicki

This comment has been minimized.

Show comment
Hide comment
@moscicki

moscicki Oct 9, 2015

Contributor

Yes. Definitely a good idea. The question is what it means that the sync failed. I guess it only means for example that the client failed to connect to the server (so there was a critical error which prevented the sync to run). But probably a sync run for which there was an error downloading one file out of 100 is OK. Or isn't it?

Contributor

moscicki commented Oct 9, 2015

Yes. Definitely a good idea. The question is what it means that the sync failed. I guess it only means for example that the client failed to connect to the server (so there was a critical error which prevented the sync to run). But probably a sync run for which there was an error downloading one file out of 100 is OK. Or isn't it?

@dragotin dragotin added this to the 2.1-next milestone Oct 10, 2015

@dragotin dragotin self-assigned this Oct 10, 2015

@dragotin

This comment has been minimized.

Show comment
Hide comment
@dragotin

dragotin Oct 10, 2015

Contributor

@moscicki yes, that's a fine line. I think I agree with what you said above, as the fail of a sync might be part of an successful sync.

Contributor

dragotin commented Oct 10, 2015

@moscicki yes, that's a fine line. I think I agree with what you said above, as the fail of a sync might be part of an successful sync.

@nickvergessen

This comment has been minimized.

Show comment
Hide comment
@nickvergessen

nickvergessen Oct 23, 2015

Contributor

Okay at least the return codes of --help and --version should be changed.
They currently return 1 (non-zero) instead of zero

also multiple options can't be combined appearantly? I tried --trust --version:

nickv@nickv-think ~ $ /usr/bin/owncloudcmd --trust --version
Source dir '--trust/' does not exist.
Contributor

nickvergessen commented Oct 23, 2015

Okay at least the return codes of --help and --version should be changed.
They currently return 1 (non-zero) instead of zero

also multiple options can't be combined appearantly? I tried --trust --version:

nickv@nickv-think ~ $ /usr/bin/owncloudcmd --trust --version
Source dir '--trust/' does not exist.
@ogoffart

This comment has been minimized.

Show comment
Hide comment
@ogoffart

ogoffart Oct 28, 2015

Collaborator

After a sync it always return 0. It doe snot matter if the sync success or fails.

We could change that, but under which criterias?
Is any SoftError a failure? or is failures oinly FatalError?

Collaborator

ogoffart commented Oct 28, 2015

After a sync it always return 0. It doe snot matter if the sync success or fails.

We could change that, but under which criterias?
Is any SoftError a failure? or is failures oinly FatalError?

@guruz guruz added the Discussion label Nov 3, 2015

@guruz guruz modified the milestones: 2.1.2, 2.1-next Nov 3, 2015

@schnidrig

This comment has been minimized.

Show comment
Hide comment
@schnidrig

schnidrig Nov 17, 2015

What about this:
0: no error at all. All files synchend and up to date.

Then if some kind of error occurs, use specific values to indicate what it was.
For example:
10: connection error
11: server gave us some 5XX http return code

20: some files failed to download

30: some files failed to upload

40: ... you name it

What about this:
0: no error at all. All files synchend and up to date.

Then if some kind of error occurs, use specific values to indicate what it was.
For example:
10: connection error
11: server gave us some 5XX http return code

20: some files failed to download

30: some files failed to upload

40: ... you name it

@dragotin dragotin modified the milestones: 2.1.2-next, 2.1.1-current Jan 8, 2016

@dragotin dragotin modified the milestones: 2.2-next, 2.1.2-next Jan 21, 2016

@dragotin dragotin assigned ogoffart and unassigned dragotin Feb 24, 2016

@guruz guruz removed this from the 2.2.0-current milestone Mar 18, 2016

@guruz guruz changed the title from Is the return code of owncloudcmd reliable? to Make owncloudcmd return code depend on sync result Mar 18, 2016

ogoffart added a commit that referenced this issue Mar 31, 2017

@SamuAlfageme

This comment has been minimized.

Show comment
Hide comment
@SamuAlfageme

SamuAlfageme Apr 10, 2017

Contributor

I'm playing around with #5669, and noticed that network errors do not stop the program after they happen:

e.g.: Authentication Failed:

$ cmd_return_codes owncloudcmd . demo.owncloud.com
Please enter user name: asdasd
Password for user asdasd: !!! OCC::JsonApiJob created for "http://demo.owncloud.com" + "ocs/v1.php/cloud/capabilities" ""
Redirecting "GET" QUrl("http://demo.owncloud.com/ocs/v1.php/cloud/capabilities?format=json") QUrl("https://demo.owncloud.com:443/ocs/v1.php/cloud/capabilities?format=json")
Stop request: Authentication failed for  "https://demo.owncloud.com:443/ocs/v1.php/cloud/capabilities?format=json"
void OCC::AbstractNetworkJob::slotFinished() QNetworkReply::NetworkError(OperationCanceledError) "Operation canceled" QVariant(Invalid)
Network error:  "ocs/v1.php/cloud/capabilities" "Operation canceled" QVariant(Invalid)
Server capabilities QVariant(Invalid)
Set proxy configuration to use NO proxy
[9] csync_exclude_load  Adding entry: *~
[...]

There are 71058518016 bytes available at "/Users/salfageme/Downloads/cmd_return_codes/" and at least 50000000 are required
===== new sync (no sync journal exists)
"===== Using Qt 5.6.2 SSL library OpenSSL 1.0.2j  26 Sep 2016 on macOS Sierra (10.12)"
sqlite3 version "3.16.1"
sqlite3 journal_mode= "wal"
bool OCC::SyncJournalDb::checkConnect() possibleUpgradeFromMirall_1_5 detected!
void OCC::SyncJournalDb::commitInternal(const QString &, bool) Transaction commit  "checkConnect" and starting new transaction
[...]
void OCC::SyncJournalDb::commitInternal(const QString &, bool) Transaction commit  "checkConnect End"
void OCC::SyncEngine::startSync() ====NOT Using Selective Sync
#### Discovery start #################################################### >>
=====Server "" "rootEtagChangesNotOnlySubFolderEtags=0"
[...]

Redirecting "PROPFIND" QUrl("http://demo.owncloud.com/remote.php/webdav/") QUrl("https://demo.owncloud.com:443/remote.php/webdav/")
Stop request: Authentication failed for  "https://demo.owncloud.com:443/remote.php/webdav/"
void OCC::AbstractNetworkJob::slotFinished() QNetworkReply::NetworkError(OperationCanceledError) "Operation canceled" QVariant(Invalid)
void OCC::DiscoverySingleDirectoryJob::lsJobFinishedWithErrorSlot(QNetworkReply *) "Operation canceled" 0 QNetworkReply::NetworkError(OperationCanceledError)
void OCC::DiscoveryMainThread::singleDirectoryJobFinishedWithErrorSlot(int, const QString &) 5 "Operation canceled"
OCC::DiscoveryJob(0x7f9c8d7076a0)  ...Returned from main thread
5 when opening  msg= "Operation canceled"
[4] csync_ftw  opendir failed for  - errno 5
[6] csync_statedb_close  sqlite3_close=0
 #### ERROR during  csync_update :  "An error occurred while opening a folder Operation canceled"
void OCC::SyncJournalDb::close() "/Users/salfageme/Downloads/cmd_return_codes/._sync_758e987b5cb0.db"
No database Transaction to commit
CSync run took  895
void OCC::SyncEngine::abort() QObject(0x0)
void OCC::SyncJournalDb::close() "/Users/salfageme/Downloads/cmd_return_codes/._sync_758e987b5cb0.db"
No database Transaction to commit 

$ echo $?
1

Instead of stopping when the first Network error pops up, it continues with the sync, even creating an empty sync journal; @ogoffart could this be prevented?

Also @schnidrig proposal on #3936 (comment) could be very useful for triaging when using owncloudcmd for automated tests

Contributor

SamuAlfageme commented Apr 10, 2017

I'm playing around with #5669, and noticed that network errors do not stop the program after they happen:

e.g.: Authentication Failed:

$ cmd_return_codes owncloudcmd . demo.owncloud.com
Please enter user name: asdasd
Password for user asdasd: !!! OCC::JsonApiJob created for "http://demo.owncloud.com" + "ocs/v1.php/cloud/capabilities" ""
Redirecting "GET" QUrl("http://demo.owncloud.com/ocs/v1.php/cloud/capabilities?format=json") QUrl("https://demo.owncloud.com:443/ocs/v1.php/cloud/capabilities?format=json")
Stop request: Authentication failed for  "https://demo.owncloud.com:443/ocs/v1.php/cloud/capabilities?format=json"
void OCC::AbstractNetworkJob::slotFinished() QNetworkReply::NetworkError(OperationCanceledError) "Operation canceled" QVariant(Invalid)
Network error:  "ocs/v1.php/cloud/capabilities" "Operation canceled" QVariant(Invalid)
Server capabilities QVariant(Invalid)
Set proxy configuration to use NO proxy
[9] csync_exclude_load  Adding entry: *~
[...]

There are 71058518016 bytes available at "/Users/salfageme/Downloads/cmd_return_codes/" and at least 50000000 are required
===== new sync (no sync journal exists)
"===== Using Qt 5.6.2 SSL library OpenSSL 1.0.2j  26 Sep 2016 on macOS Sierra (10.12)"
sqlite3 version "3.16.1"
sqlite3 journal_mode= "wal"
bool OCC::SyncJournalDb::checkConnect() possibleUpgradeFromMirall_1_5 detected!
void OCC::SyncJournalDb::commitInternal(const QString &, bool) Transaction commit  "checkConnect" and starting new transaction
[...]
void OCC::SyncJournalDb::commitInternal(const QString &, bool) Transaction commit  "checkConnect End"
void OCC::SyncEngine::startSync() ====NOT Using Selective Sync
#### Discovery start #################################################### >>
=====Server "" "rootEtagChangesNotOnlySubFolderEtags=0"
[...]

Redirecting "PROPFIND" QUrl("http://demo.owncloud.com/remote.php/webdav/") QUrl("https://demo.owncloud.com:443/remote.php/webdav/")
Stop request: Authentication failed for  "https://demo.owncloud.com:443/remote.php/webdav/"
void OCC::AbstractNetworkJob::slotFinished() QNetworkReply::NetworkError(OperationCanceledError) "Operation canceled" QVariant(Invalid)
void OCC::DiscoverySingleDirectoryJob::lsJobFinishedWithErrorSlot(QNetworkReply *) "Operation canceled" 0 QNetworkReply::NetworkError(OperationCanceledError)
void OCC::DiscoveryMainThread::singleDirectoryJobFinishedWithErrorSlot(int, const QString &) 5 "Operation canceled"
OCC::DiscoveryJob(0x7f9c8d7076a0)  ...Returned from main thread
5 when opening  msg= "Operation canceled"
[4] csync_ftw  opendir failed for  - errno 5
[6] csync_statedb_close  sqlite3_close=0
 #### ERROR during  csync_update :  "An error occurred while opening a folder Operation canceled"
void OCC::SyncJournalDb::close() "/Users/salfageme/Downloads/cmd_return_codes/._sync_758e987b5cb0.db"
No database Transaction to commit
CSync run took  895
void OCC::SyncEngine::abort() QObject(0x0)
void OCC::SyncJournalDb::close() "/Users/salfageme/Downloads/cmd_return_codes/._sync_758e987b5cb0.db"
No database Transaction to commit 

$ echo $?
1

Instead of stopping when the first Network error pops up, it continues with the sync, even creating an empty sync journal; @ogoffart could this be prevented?

Also @schnidrig proposal on #3936 (comment) could be very useful for triaging when using owncloudcmd for automated tests

@guruz guruz removed the ReadyToTest label Apr 10, 2017

@ogoffart

This comment has been minimized.

Show comment
Hide comment
@ogoffart

ogoffart Apr 10, 2017

Collaborator

@SamuAlfageme i'm not sure what's the issue here. 1 means error.
And the program stops when the sync stops.

But yes, it's true that a failure to query the capabilities could abort earlier But that's another issue.

Edit: spinned #5692 out of this issue

Collaborator

ogoffart commented Apr 10, 2017

@SamuAlfageme i'm not sure what's the issue here. 1 means error.
And the program stops when the sync stops.

But yes, it's true that a failure to query the capabilities could abort earlier But that's another issue.

Edit: spinned #5692 out of this issue

@SamuAlfageme

This comment has been minimized.

Show comment
Hide comment
@SamuAlfageme

SamuAlfageme Apr 11, 2017

Contributor

@ogoffart yeah, I was not talking about an issue with the return code itself but the program's flow after the error occurred, sorry if I wasn't clear and it didn't belong here.

Thanks for opening the new issue! I'll close this one after testing out a couple more scenarios.

Contributor

SamuAlfageme commented Apr 11, 2017

@ogoffart yeah, I was not talking about an issue with the return code itself but the program's flow after the error occurred, sorry if I wasn't clear and it didn't belong here.

Thanks for opening the new issue! I'll close this one after testing out a couple more scenarios.

@SamuAlfageme SamuAlfageme added this to the 2.4.0 milestone Apr 11, 2017

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