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

libsync: use the new webdav url if the server reports it #5332

Merged
merged 1 commit into from
Nov 29, 2016

Conversation

ogoffart
Copy link
Contributor

The rules to select the webdav url are now:

  • If the server reports that the new chunking algorithm is working,
    always use remote.php/dav/files/
    This capability can be overriden with an environment variable

  • Otherwise, use the dav path provided by the theme, which defaults to
    remote.php/webdav

This means that with the newer server, the branding can no longer override
the webdav URL. If there is still an usecase for the branding to do so, we
need to find another way to override it. But it is now more complicated to
configure as might need include the username and need different endpoint
depending on the operations (chunks or not)

@mention-bot
Copy link

@ogoffart, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ckamm, @dragotin and @danimo to be potential reviewers.

@@ -57,6 +57,11 @@ Account::~Account()

QString Account::davPath() const
{
if (capabilities().chunkingNg()) {
Copy link
Contributor

@mrow4a mrow4a Nov 21, 2016

Choose a reason for hiding this comment

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

Why do you need this "if" here? I might want to use it for bundling also. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Chunking ng is in >=9.2..

Copy link
Contributor

@mrow4a mrow4a Nov 22, 2016

Choose a reason for hiding this comment

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

EDIT: oh sorry, code in github wrapped and have not seen the rest. Of course it is good.

Copy link
Contributor

@mrow4a mrow4a Nov 22, 2016

Choose a reason for hiding this comment

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

But been mostly thinking about creating just separate function instead of "if", but in this case it makes no sense. It is all good. For bundling I will just need to work-around this. Because I would need that String (the same) anyways, but only for uploaded files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because both chunkingng and the new dav url need to be enabled at the same time! (the old chunking don't work with the new url, and the new chunking don't work with the old one)

Maybe the capability should be renamed "useNewDavPath()"

Copy link
Contributor

Choose a reason for hiding this comment

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

"filesDavPath" is ok with me.

@guruz guruz added this to the 2.3.0 milestone Nov 22, 2016
@guruz guruz assigned ogoffart and unassigned guruz Nov 22, 2016
@guruz
Copy link
Contributor

guruz commented Nov 22, 2016

👎 for now. Can you mention the issue ID in the commit?
#4007

@michaelstingl @rperezb Please see the comment in the commit message
c659a9f
Is there still a use case to change the DAV url in branding?

👍 if this is clarified

@rperezb
Copy link

rperezb commented Nov 23, 2016

@guruz in ownBrander we do have a variable for the users to update the webdav end point:

  • webDavPath()

If i got this right, within the version 2.3.0, this is not going to be possible any more,
@michaelstingl do we have any customers with the desktop_webdav_url_text modified? If so, we need to handle this before updating ownBrander

@mrow4a
Copy link
Contributor

mrow4a commented Nov 24, 2016

When merge is planned? Is it possible to create new function filesDavPath()?

@ogoffart
Copy link
Contributor Author

@rperezb , @michaelstingl : With the client 2.3.0 , the webDavPath ownbrander setting will only have any impact if the server is < 9.2

From owncloud 9.2, we will use the new dav system with a new URL scheme. If we want to keep an option, we need to make it smarter so we can configure it for all the possible end point.

Depending on the use case of this setting, i suggest one of the following:

  • We realize that the reason that forced us to create this settings are no longer applicable with the new scheme, and we drop the option
  • If a custom webdav url is configured, we do not enable the new url scheme (This also means the chunking NG and bundling will not be enabled)
  • We find another way to configure the webdav path

@rperezb
Copy link

rperezb commented Nov 28, 2016

about how to proceed, let's see what @hodyroff or @felixboehm thinks, at the end, this was a requirement from customers.

to sum up, we do have an option on the desktop client that allows clients to modify the webdav end point, instead of using the one by default (remote.php/webdav) is this needed to any customer? / do we want to keep this?
for context, this is on the backlog for mobile apps, still not implented nor planned

The rules to select the webdav url are now:

 - If the server reports that the new chunking algorithm is working,
   always use remote.php/dav/files/<username>
   This capability can be overriden with an environment variable

 - Otherwise, use the dav path provided by the theme, which defaults to
   remote.php/webdav

This means that with the newer server, the branding can no longer override
the webdav URL. If there is still an usecase for the branding to do so, we
need to find another way to override it. But it is now more complicated to
configure as might need include the username and need different endpoint
depending on the operations (chunks or not)

Issue #4007
@ogoffart
Copy link
Contributor Author

The option will still be working with server older than 9.2

@felixboehm
Copy link
Contributor

I can't tell if we need to be able to configure webdav routes.
But we should document this, right? @owncloud/doc

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

Successfully merging this pull request may close these issues.

None yet

7 participants