Merge lp:~carlalex/duplicity/duplicity into lp:~duplicity-team/duplicity/0.8-series

looks good! and man page is adapted as well. i like it.

there's just one issue that i'd like to change. switching actual backend via --parameter is deprecated since we adopted the stacked scheme. you can check eg.

ssh_paramiko_backend.py
ssh_pexpect_backend.py

for implementations where two backends provide implementations for the same backends (sftp,scp).
switching would then work via

boto3+s3://
while default will stay
boto+s3// equaling s3://

while i'd prefer the above i'd not insist on it, if you can't find te time. thanks for this contribution again!.. ede/duply.net

I'll look into implementing boto3+s3. I have mixed feelings on it. On the plus side, I like that a URL should always behave one way. On the minus, I would think it someday should become the default for s3. But, implementing a non-default boto3+s3 now doesn't preclude changing the defaults in the future. I expect boto will die completely some day.

> looks good! and man page is adapted as well. i like it.
>
> there's just one issue that i'd like to change. switching actual backend via
> --parameter is deprecated since we adopted the stacked scheme. you can check
> eg.
>
> ssh_paramiko_backend.py
> ssh_pexpect_backend.py
>
> for implementations where two backends provide implementations for the same
> backends (sftp,scp).
> switching would then work via
>
> boto3+s3://
> while default will stay
> boto+s3// equaling s3://
>
> while i'd prefer the above i'd not insist on it, if you can't find te time.
> thanks for this contribution again!.. ede/duply.net

> On the minus, I would think it someday should become the default for s3. But, implementing a non-default boto3+s3 now doesn't preclude changing the defaults in the future. I expect boto will die completely some day.

that's the beauty of switching backends via scheme. moving the default later simply means add s3:// to te new default backend and remove it from the old, which then can still be selected by the prefixed scheme (if needed). eg.

moving

duplicity.backend.register_backend(u"sftp", SSHParamikoBackend)
duplicity.backend.register_backend(u"scp", SSHParamikoBackend)

from paramiko back to pexpect would make the older legacy pexpect backend default again. simple as that.

..ede

refactored to using scheme to select backend. Still should add some test code.

Not ready for merge. There are test failures in testing/manual/backendtest

So, the test failures in manual/backendtest are in test_delete and test_list. I think the backend is actually listing and deleting as I would expect, but the test is failing due to a type mismatch. These tests as written are looking for files names b'a' and b'b', but list is returning then as regular (unicode?) strings, not byte strings.

In this case, I am not sure if the test is wrong, or if I should change the backend to return byte strings in list rather than unicode strings.

Yes, it needs to be bytes. Use util.fsencode() to convert.

On Sun, Dec 1, 2019 at 5:02 PM carlalex <email address hidden> wrote:

> So, the test failures in manual/backendtest are in test_delete and
> test_list. I think the backend is actually listing and deleting as I would
> expect, but the test is failing due to a type mismatch. These tests as
> written are looking for files names b'a' and b'b', but list is returning
> then as regular (unicode?) strings, not byte strings.
>
> In this case, I am not sure if the test is wrong, or if I should change
> the backend to return byte strings in list rather than unicode strings.
>
> --
> https://code.launchpad.net/~carlalex/duplicity/duplicity/+merge/376206
> You are subscribed to branch lp:duplicity.
>

hey Ken,

i see that for other backends this is done automagically in
https://bazaar.launchpad.net/~duplicity-team/duplicity/0.8-series/view/head:/duplicity/backend.py#L562

can any of you see a reason why it is not for the boto3 backend?.. ede

On 02.12.2019 15:12, Kenneth Loafman wrote:
> Yes, it needs to be bytes. Use util.fsencode() to convert.
>
>
>
> On Sun, Dec 1, 2019 at 5:02 PM carlalex <email address hidden> wrote:
>
>> So, the test failures in manual/backendtest are in test_delete and
>> test_list. I think the backend is actually listing and deleting as I would
>> expect, but the test is failing due to a type mismatch. These tests as
>> written are looking for files names b'a' and b'b', but list is returning
>> then as regular (unicode?) strings, not byte strings.
>>
>> In this case, I am not sure if the test is wrong, or if I should change
>> the backend to return byte strings in list rather than unicode strings.
>>
>> --
>> https://code.launchpad.net/~carlalex/duplicity/duplicity/+merge/376206
>> You are subscribed to branch lp:duplicity.
>>
>

Don't know. I know that duplicity requires bytes. I have not used the
manual backend test in a while, so it may be out of date.

On Mon, Dec 2, 2019 at 8:21 AM edso <email address hidden> wrote:

> hey Ken,
>
> i see that for other backends this is done automagically in
>
> https://bazaar.launchpad.net/~duplicity-team/duplicity/0.8-series/view/head:/duplicity/backend.py#L562
>
> can any of you see a reason why it is not for the boto3 backend?.. ede
>
>
> On 02.12.2019 15:12, Kenneth Loafman wrote:
> > Yes, it needs to be bytes. Use util.fsencode() to convert.
> >
> >
> >
> > On Sun, Dec 1, 2019 at 5:02 PM carlalex <email address hidden> wrote:
> >
> >> So, the test failures in manual/backendtest are in test_delete and
> >> test_list. I think the backend is actually listing and deleting as I
> would
> >> expect, but the test is failing due to a type mismatch. These tests as
> >> written are looking for files names b'a' and b'b', but list is returning
> >> then as regular (unicode?) strings, not byte strings.
> >>
> >> In this case, I am not sure if the test is wrong, or if I should change
> >> the backend to return byte strings in list rather than unicode strings.
> >>
> >> --
> >> https://code.launchpad.net/~carlalex/duplicity/duplicity/+merge/376206
> >> You are subscribed to branch lp:duplicity.
> >>
> >
>
>
> --
> https://code.launchpad.net/~carlalex/duplicity/duplicity/+merge/376206
> You are subscribed to branch lp:duplicity.
>

maybe this helps

https://bazaar.launchpad.net/~duplicity-team/duplicity/0.8-series/view/head:/testing/manual/backendtest#L60
should probably use 'get_backend' instead of 'get_backend_object'
https://bazaar.launchpad.net/~duplicity-team/duplicity/0.8-series/view/head:/duplicity/backend.py#L216
which properly wraps the backend in BackendWrapper()

like it is done in current duplicity, see
https://bazaar.launchpad.net/~duplicity-team/duplicity/0.8-series/view/head:/duplicity/commandline.py#L1043

..ede/duply.net

On 02.12.2019 17:57, Kenneth Loafman wrote:
> Don't know. I know that duplicity requires bytes. I have not used the
> manual backend test in a while, so it may be out of date.
>
> On Mon, Dec 2, 2019 at 8:21 AM edso <email address hidden> wrote:
>
>> hey Ken,
>>
>> i see that for other backends this is done automagically in
>>
>> https://bazaar.launchpad.net/~duplicity-team/duplicity/0.8-series/view/head:/duplicity/backend.py#L562
>>
>> can any of you see a reason why it is not for the boto3 backend?.. ede
>>
>>
>> On 02.12.2019 15:12, Kenneth Loafman wrote:
>>> Yes, it needs to be bytes. Use util.fsencode() to convert.
>>>
>>>
>>>
>>> On Sun, Dec 1, 2019 at 5:02 PM carlalex <email address hidden> wrote:
>>>
>>>> So, the test failures in manual/backendtest are in test_delete and
>>>> test_list. I think the backend is actually listing and deleting as I
>> would
>>>> expect, but the test is failing due to a type mismatch. These tests as
>>>> written are looking for files names b'a' and b'b', but list is returning
>>>> then as regular (unicode?) strings, not byte strings.
>>>>
>>>> In this case, I am not sure if the test is wrong, or if I should change
>>>> the backend to return byte strings in list rather than unicode strings.
>>>>
>>>> --
>>>> https://code.launchpad.net/~carlalex/duplicity/duplicity/+merge/376206
>>>> You are subscribed to branch lp:duplicity.
>>>>
>>>
>>
>>
>> --
>> https://code.launchpad.net/~carlalex/duplicity/duplicity/+merge/376206
>> You are subscribed to branch lp:duplicity.
>>
>

Looks like manual.backendtest is way out of date. I think this needs to be moved to functional tests and incorporated into tox testing.

> Looks like manual.backendtest is way out of date. I think this needs to be
> moved to functional tests and incorporated into tox testing.

Where does that leave the merge request? What else should be done?

On 03.12.2019 05:06, carlalex wrote:
>> Looks like manual.backendtest is way out of date. I think this needs to be
>> moved to functional tests and incorporated into tox testing.
>
> Where does that leave the merge request? What else should be done?
>

assuming you tested with a live duplicity as well and as other live backends returning the list as strings work properly, i'd say it's fine in this regard.

one thing still, using the prefixed scheme you should probably update man page section section 'Url Format' similar as it is already done for ssh backends.

also could you please add 'boto+s3' to botobackend.py (register_backend() & uses_netloc.extend()) while your at it? just for completeness.

thanks! ..ede/duply.net

Testing a live backup against S3, I don't see any difference in list returning strings or byte strings. The only difference I've seen is in the manual test.

I had added register_backend to s3_boto3_backend, keeping it self contained and following the convention in the ssh backends. The backends do not seem entirely consistent on this point, with ssh having the two flavors entirely self contained, and boto and cf separating the implementation from the registration. The primary advantage of separating the registration from the implementation of the backend appears to be selecting backend implementation by CLI option, which I was told was now discouraged. FWIW, I'd say "boto" is not correct for this new backend anyway, since boto3 is really a completely different library, which can coexist with boto in a project. If we do want to register the new backend along side the older s3 backends in a common location, I'd suggest something named "s3" over "boto", reflecting the backup server type rather than the particular implementation.

I didn't register a netloc. Per the comments in backend.py, that didn't seem correct since the new backend doesn't have a network location. The new URL follows the behavior of the older "s3+http", which is also not in the netloc list.

I had already added boto3+s3 to the url scheme section and an extended explanation under "A note on amazon s3" in my latest updates, so I'm not sure what else you are asking for. Is the bzr merge request not up to date?

> Testing a live backup against S3, I don't see any difference in list returning
> strings or byte strings. The only difference I've seen is in the manual test.

that's because in live duplicity the backend are wrapped in BackendWrapper class. see comment https://code.launchpad.net/~carlalex/duplicity/duplicity/+merge/376206/comments/985749

> I had added register_backend to s3_boto3_backend, keeping it self contained
> and following the convention in the ssh backends. The backends do not seem
> entirely consistent on this point, with ssh having the two flavors entirely
> self contained, and boto and cf separating the implementation from the
> registration.

they are not. they were implemented before we switched to the prefixed scheme approach.

>The primary advantage of separating the registration from the
> implementation of the backend appears to be selecting backend implementation
> by CLI option, which I was told was now discouraged. FWIW, I'd say "boto"
> is not correct for this new backend anyway, since boto3 is really a completely
> different library, which can coexist with boto in a project. If we do want to
> register the new backend along side the older s3 backends in a common
> location, I'd suggest something named "s3" over "boto", reflecting the backup
> server type rather than the particular implementation.

not sure what you mean here.

>
> I didn't register a netloc. Per the comments in backend.py, that didn't seem
> correct since the new backend doesn't have a network location. The new URL
> follows the behavior of the older "s3+http", which is also not in the netloc
> list.

i was talking about the older boto backend here (note: i wrote botobackend.py) . your implementation seems not to use netloc indeed.

> I had already added boto3+s3 to the url scheme section and an extended
> explanation under "A note on amazon s3" in my latest updates, so I'm not sure
> what else you are asking for. Is the bzr merge request not up to date?

in the man page there is a section 'Url Format' that explains the url formats per backend (meaning protocol) currently it looks like

-->

URL Format

Duplicity uses the URL format (as standard as possible) to define data locations. The generic format for a URL is:
scheme://[user[:password]@]host[:port]/[/]path
It is not ....

[SNIP]

S3 storage (Amazon)

s3://host[:port]/bucket_name[/prefix]
s3+http://bucket_name[/prefix]
See also A NOTE ON EUROPEAN S3 BUCKETS

SCP/SFTP access

scp://.. or
sftp://user[:password]@other.host[:port]/[relative|/absolute]_path
defaults are paramiko+scp:// and paramiko+sftp://
alternatively try pexpect+scp://, pexpect+sftp://, lftp+sftp://
See also --ssh-askpass, --ssh-options and A NOTE ON SSH BACKENDS.

[SNIP]

<--

see how the alternate backends are documented for scp/sftp? same would be advisable for s3, now that we have two backend (implementations) that provide S3 access.

if you don't want to touch the older botobackend.py i'm fine with that of course.

thanks! ..ede/duply.net

> > I had added register_backend to s3_boto3_backend, keeping it self contained
> > and following the convention in the ssh backends. The backends do not seem
> > entirely consistent on this point, with ssh having the two flavors entirely
> > self contained, and boto and cf separating the implementation from the
> > registration.
>
> they are not. they were implemented before we switched to the prefixed scheme
> approach.
>
> >The primary advantage of separating the registration from the
> > implementation of the backend appears to be selecting backend implementation
> > by CLI option, which I was told was now discouraged. FWIW, I'd say "boto"
> > is not correct for this new backend anyway, since boto3 is really a
> completely
> > different library, which can coexist with boto in a project. If we do want
> to
> > register the new backend along side the older s3 backends in a common
> > location, I'd suggest something named "s3" over "boto", reflecting the
> backup
> > server type rather than the particular implementation.
>
> not sure what you mean here.
>

Two things: 1) registering in botobackend.py as requested seems to conflict with the request to follow the newer prefix conventions (where the example of SSH registers each in their own ssh_<backend>.py). I followed the SSH convention when I renamed it the new backend s3_boto3_backend.py. 2) If i do register the all the s3 backends in a common py file, calling that py file "boto" isn't right for the new one - boto and boto3 are completely separate. If that's the registration convention you want to follow, I'd suggest "s3backend.py", which would collect the two older "boto" backends, and the newer "boto3" backend. But, I don't see the point of breaking the encapsulation of "everything in the new backend is in the new backend file, including registration", which is what ssh and most non-prefixed backends do.

>
> > I had already added boto3+s3 to the url scheme section and an extended
> > explanation under "A note on amazon s3" in my latest updates, so I'm not
> sure
> > what else you are asking for. Is the bzr merge request not up to date?
>
> in the man page there is a section 'Url Format' that explains the url formats
> per backend (meaning protocol) currently it looks like
>
> -->
>
> URL Format
>
> Duplicity uses the URL format (as standard as possible) to define data
> locations. The generic format for a URL is:
> scheme://[user[:password]@]host[:port]/[/]path
> It is not ....
>
> [SNIP]
>
> S3 storage (Amazon)
>
> s3://host[:port]/bucket_name[/prefix]
> s3+http://bucket_name[/prefix]
> See also A NOTE ON EUROPEAN S3 BUCKETS
>

I think you are looking at an old diff. I updated that when I switched from --s3-use-boto3 to boto3+s3. Did I need to do more than push my change to my branch to update the merge request? (First project that I've used bzr in...)

Yes, just push the changes and let us know.

Thanks for the fixes!

On 04.12.2019 16:49, Carl A. Adams wrote:
>>> I had added register_backend to s3_boto3_backend, keeping it self contained
>>> and following the convention in the ssh backends. The backends do not seem
>>> entirely consistent on this point, with ssh having the two flavors entirely
>>> self contained, and boto and cf separating the implementation from the
>>> registration.
>>
>> they are not. they were implemented before we switched to the prefixed scheme
>> approach.
>>
>>> The primary advantage of separating the registration from the
>>> implementation of the backend appears to be selecting backend implementation
>>> by CLI option, which I was told was now discouraged. FWIW, I'd say "boto"
>>> is not correct for this new backend anyway, since boto3 is really a
>> completely
>>> different library, which can coexist with boto in a project. If we do want
>> to
>>> register the new backend along side the older s3 backends in a common
>>> location, I'd suggest something named "s3" over "boto", reflecting the
>> backup
>>> server type rather than the particular implementation.
>>
>> not sure what you mean here.
>>
>
>
> Two things: 1) registering in botobackend.py as requested seems to conflict with the request to follow the newer prefix conventions (where the example of SSH registers each in their own ssh_<backend>.py). I followed the SSH convention when I renamed it the new backend s3_boto3_backend.py. 2) If i do register the all the s3 backends in a common py file, calling that py file "boto" isn't right for the new one - boto and boto3 are completely separate. If that's the registration convention you want to follow, I'd suggest "s3backend.py", which would collect the two older "boto" backends, and the newer "boto3" backend. But, I don't see the point of breaking the encapsulation of "everything in the new backend is in the new backend file, including registration", which is what ssh and most non-prefixed backends do.
>

no worries. i think we are still misunderstanding each other. doesn't matter though! just leave the botobackend as is and i'll do the changes when i find the time :)

>>
>>> I had already added boto3+s3 to the url scheme section and an extended
>>> explanation under "A note on amazon s3" in my latest updates, so I'm not
>> sure
>>> what else you are asking for. Is the bzr merge request not up to date?
>>
>> in the man page there is a section 'Url Format' that explains the url formats
>> per backend (meaning protocol) currently it looks like
>>
>> -->
>>
>> URL Format
>>
>> Duplicity uses the URL format (as standard as possible) to define data
>> locations. The generic format for a URL is:
>> scheme://[user[:password]@]host[:port]/[/]path
>> It is not ....
>>
>> [SNIP]
>>
>> S3 storage (Amazon)
>>
>> s3://host[:port]/bucket_name[/prefix]
>> s3+http://bucket_name[/prefix]
>> See also A NOTE ON EUROPEAN S3 BUCKETS
>>
>
>
> I think you are looking at an old diff. I updated that when I switched from --s3-use-boto3 to boto3+s3. Did I need to do more than push my change to my branch to update the merge request? (First project that I've used bzr in...)
>
>
>

ok, i see it now. fine by me then!.. ede/duply.net

> no worries. i think we are still misunderstanding each other. doesn't matter
> though! just leave the botobackend as is and i'll do the changes when i find
> the time :)
>

That seems likely. It'll be apparent when I see the final change. Thanks for your time.

> Yes, just push the changes and let us know.
>
> Thanks for the fixes!

The dumb "I've never worked with bzr" question... What do I need to do other than have all the changes in my branch? There is already a merge request outstanding between my branch and lp:duplicity

Thanks for merging.