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

Lazy initialize external storages #11694

Merged
merged 1 commit into from
Oct 22, 2014
Merged

Lazy initialize external storages #11694

merged 1 commit into from
Oct 22, 2014

Conversation

PVince81
Copy link
Contributor

Fixed the following external storages to not connect in the constructor,
but do it on-demand when getConnection() is called.

  • S3
  • SWIFT
  • SFTP

Prerequisite for the fix of #10820

Please review/test @butonic @bantu @icewind1991

I tested SFTP (manual+unit tests) and SWIFT (manual+unit tests against devstack) and this change did not break them.

@icewind1991
Copy link
Contributor

Code looks good 👍

@LukasReschke
Copy link
Member

Rebase please.

Fixed the following external storages to not connect in the constructor,
but do it on-demand when getConnection() is called.
- S3
- SWIFT
- SFTP
@PVince81 PVince81 force-pushed the extstorage-lazyinit branch from fa65511 to 075e8d8 Compare October 22, 2014 10:42
@PVince81
Copy link
Contributor Author

Rebased

@scrutinizer-notifier
Copy link

The inspection completed: 9 new issues, 8 updated code elements

@ghost
Copy link

ghost commented Oct 22, 2014

🚀 Test PASSed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/896/
🚀 Test PASSed. 🚀

@LukasReschke
Copy link
Member

👍 🎫

LukasReschke added a commit that referenced this pull request Oct 22, 2014
@LukasReschke LukasReschke merged commit fe912ca into master Oct 22, 2014
@LukasReschke LukasReschke deleted the extstorage-lazyinit branch October 22, 2014 12:12
@PVince81
Copy link
Contributor Author

@karlitschek backport to stable7 ? It only affects SFTP, SWIFT and S3 and is a prerequisite for #10820

@karlitschek
Copy link
Contributor

yes. please backport

@PVince81
Copy link
Contributor Author

I'm on it. There are some conflicts.

@PVince81
Copy link
Contributor Author

Backported to stable7 as 09d80ac
I also re-ran the unit tests for SWIFT and SFTP.

@butonic
Copy link
Member

butonic commented Oct 22, 2014

💣 💣💣 completely breaks s3 💣 💣 💣
$base_url and $param is not remembered as a global variable (and thun used in the connection)

have not had a look at swift or SFTP

please be more thorough when testing files external stuff ...

@LukasReschke
Copy link
Member

Filed #11714 to track this properly.

Apologize for missing that on the review, shouldn't have happened and will try hard to prevent such failures in the future. - Seems like I should setup a S3 account...
This is clearly not only @PVince81's fault but also I and @icewind1991 failed here.

Thanks a lot for catching that and sorry again. - I'm just annoyed about this error as you are.

@PVince81
Copy link
Contributor Author

I didn't test S3. The US buckets are way too slow...
Will fix the issue.

@LukasReschke
Copy link
Member

Any chance to have the external storage tests also running on Jenkins? - Wouldn't even have to run on all PRs but at least on the release branches it might be a nice addition.

\cc @DeepDiver1975

@lock lock bot locked as resolved and limited conversation to collaborators Aug 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants