feat(api): support file format for repository archive #1561

dAnjou · 2021-07-31T20:27:21Z

No description provided.

codecov-commenter · 2021-07-31T20:34:24Z

Codecov Report

Merging #1561 (3910048) into master (ae97196) will decrease coverage by 4.49%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #1561      +/-   ##
==========================================
- Coverage   91.16%   86.67%   -4.50%     
==========================================
  Files          74       74              
  Lines        4177     4179       +2     
==========================================
- Hits         3808     3622     -186     
- Misses        369      557     +188

Flag	Coverage Δ
cli_func_v4	`80.71% <33.33%> (-0.04%)`	⬇️
py_func_v4	`?`
unit	`82.26% <33.33%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
gitlab/v4/objects/repositories.py	`55.73% <33.33%> (-25.62%)`	⬇️
gitlab/v4/objects/files.py	`61.76% <0.00%> (-30.89%)`	⬇️
gitlab/v4/objects/milestones.py	`71.42% <0.00%> (-28.58%)`	⬇️
gitlab/v4/objects/tags.py	`63.88% <0.00%> (-25.00%)`	⬇️
gitlab/utils.py	`65.51% <0.00%> (-24.14%)`	⬇️
gitlab/v4/objects/sidekiq.py	`80.95% <0.00%> (-19.05%)`	⬇️
gitlab/v4/objects/commits.py	`78.26% <0.00%> (-15.95%)`	⬇️
gitlab/v4/objects/snippets.py	`82.92% <0.00%> (-14.64%)`	⬇️
gitlab/v4/objects/clusters.py	`85.71% <0.00%> (-14.29%)`	⬇️
... and 12 more

JohnVillalovos

This will also need a documentation update in https://python-gitlab.readthedocs.io/en/stable/gl_objects/projects.html

See "Get the repository archive:"

And can it specify the possible options from https://docs.gitlab.com/ee/api/repositories.html#get-file-archive:
tar.gz, tar.bz2, tbz, tbz2, tb2, bz2, tar, and zip.

dAnjou · 2021-08-01T10:12:18Z

Thanks, @JohnVillalovos! Kinda submitted this as a conversation starter because I also wasn't quite sure about whether and where to add tests.

Will make the changes you suggested.

dAnjou · 2021-08-01T10:46:44Z

And can it specify the possible options from https://docs.gitlab.com/ee/api/repositories.html#get-file-archive:
tar.gz, tar.bz2, tbz, tbz2, tb2, bz2, tar, and zip.

I was hesitant to explicitly list them in the method documentation because GitLab might change them, so instead I added a note with a link to the example.

nejch · 2021-08-01T19:00:27Z

And can it specify the possible options from https://docs.gitlab.com/ee/api/repositories.html#get-file-archive:
tar.gz, tar.bz2, tbz, tbz2, tb2, bz2, tar, and zip.

I was hesitant to explicitly list them in the method documentation because GitLab might change them, so instead I added a note with a link to the example.

That's a good point, maybe this is easier long-term WDYT @JohnVillalovos?

If you want to add tests - maybe extract these lines into test_repository_archive() and try adding a different format:

python-gitlab/tests/functional/api/test_repository.py

Lines 51 to 55 in 90ecf2f

    
           archive = project.repository_archive() 
        
           assert isinstance(archive, bytes) 
        
           archive2 = project.repository_archive("master") 
        
           assert archive == archive2

And maybe test that it really worked with e.g.
https://docs.python.org/3/library/tarfile.html#tarfile.is_tarfile
https://docs.python.org/3/library/zipfile.html#zipfile.is_zipfile

JohnVillalovos · 2021-08-01T19:37:41Z

And can it specify the possible options from https://docs.gitlab.com/ee/api/repositories.html#get-file-archive:
tar.gz, tar.bz2, tbz, tbz2, tb2, bz2, tar, and zip.

I was hesitant to explicitly list them in the method documentation because GitLab might change them, so instead I added a note with a link to the example.

That's a good point, maybe this is easier long-term WDYT @JohnVillalovos?

Sounds reasonable to me. Could also add in the note

As of 2021-08, the format options are: tar.gz, tar.bz2, tbz, tbz2, tb2, bz2, tar, and zip.

For the formats available, refer to
https://docs.gitlab.com/ce/api/repositories.html#get-file-archive

I am perfectly fine with the current doc update with only the pointer to the Gitlab docs.

dAnjou · 2021-08-06T14:08:36Z

I've added a test and ran it locally a few times. For some reason it always got a 404 for tar.gz and tar.bz2. No idea why, because I created a project on the GitLab instance that the tests use and getting these formats via the API worked perfectly fine.

nejch · 2021-08-22T16:22:38Z

I've added a test and ran it locally a few times. For some reason it always got a 404 for tar.gz and tar.bz2. No idea why, because I created a project on the GitLab instance that the tests use and getting these formats via the API worked perfectly fine.

@dAnjou Thanks, maybe it's something to do with URL encoding since they include . and we've had issues with this before. I'll take a look.

JohnVillalovos · 2021-12-17T20:23:34Z

Thanks @dAnjou

Can you rebase the patch rather than adding a merge commit?

JohnVillalovos · 2021-12-18T05:03:05Z

I've added a test and ran it locally a few times. For some reason it always got a 404 for tar.gz and tar.bz2. No idea why, because I created a project on the GitLab instance that the tests use and getting these formats via the API worked perfectly fine.

Strange. I wonder why that is happening. @nejch did you ever have a chance to investigate?

JohnVillalovos · 2021-12-18T05:37:55Z

Strange. I wonder why that is happening. @nejch did you ever have a chance to investigate?

I think I figured it out. See #1758 for my "fix" and can see the functional tests pass.

@nejch would like to get your feedback on this.

I really don't think that we should be converting periods to %2E as I believe we have other issues about that causing problems.

nejch · 2021-12-19T21:56:33Z

Ok, so it is encoding drama again 😅

I really don't think that we should be converting periods to %2E as I believe we have other issues about that causing problems.

Could be, would love to get rid of more custom code. Do you have a link to the other issue? We just need to make sure that this is not needed anymore:

python-gitlab/gitlab/client.py

Lines 596 to 601 in 3ee061c

    
           # Requests assumes that `.` should not be encoded as %2E and will make 
        
           # changes to urls using this encoding. Using a prepped request we can 
        
           # get the desired behavior. 
        
           # The Requests behavior is right but it seems that web servers don't 
        
           # always agree with this decision (this is the case with a default 
        
           # gitlab installation)

So maybe need to check again what a221d7b was trying to achieve. And possibly test this with paths that may include dots so we don't have a regression 🤔

Forcing the encoding of '.' to '%2E' causes issues. It also goes against the RFC: https://datatracker.ietf.org/doc/html/rfc3986.html#section-2.3 From the RFC: For consistency, percent-encoded octets in the ranges of ALPHA (%41-%5A and %61-%7A), DIGIT (%30-%39), hyphen (%2D), period (%2E), underscore (%5F), or tilde (%7E) should not be created by URI producers... Related #1006 Related #1356 Related #1561 BREAKING CHANGE: This possibly could be a breaking for users who have incorrectly configured GitLab servers which don't handle period '.' characters correctly.

Forcing the encoding of '.' to '%2E' causes issues. It also goes against the RFC: https://datatracker.ietf.org/doc/html/rfc3986.html#section-2.3 From the RFC: For consistency, percent-encoded octets in the ranges of ALPHA (%41-%5A and %61-%7A), DIGIT (%30-%39), hyphen (%2D), period (%2E), underscore (%5F), or tilde (%7E) should not be created by URI producers... Closes #1006 Related #1356 Related #1561 BREAKING CHANGE: This possibly could be a breaking for users who have incorrectly configured GitLab servers which don't handle period '.' characters correctly.

Forcing the encoding of '.' to '%2E' causes issues. It also goes against the RFC: https://datatracker.ietf.org/doc/html/rfc3986.html#section-2.3 From the RFC: For consistency, percent-encoded octets in the ranges of ALPHA (%41-%5A and %61-%7A), DIGIT (%30-%39), hyphen (%2D), period (%2E), underscore (%5F), or tilde (%7E) should not be created by URI producers... Closes #1006 Related #1356 Related #1561 BREAKING CHANGE: This could potentially be a breaking change for users who have incorrectly configured GitLab servers which don't handle period '.' characters correctly.

JohnVillalovos · 2021-12-20T02:43:59Z

Ok, so it is encoding drama again 😅

I really don't think that we should be converting periods to %2E as I believe we have other issues about that causing problems.

Could be, would love to get rid of more custom code. Do you have a link to the other issue? We just need to make sure that this is not needed anymore:

I created a PR #1766 to get rid of it and referenced the issues I found, though I believe I had seen other things related to this but didn't find them in my quick search.

So maybe need to check again what a221d7b was trying to achieve. And possibly test this with paths that may include dots so we don't have a regression 🤔

I thought we cleaned that up somewhat in d56a434 But I'm not exactly sure what you are asking for, probably because I'm a bit tired at the moment and not reading everything 😟

Not sure if you really meant this commit d7c7911 which first added that '.' -> '%2E' translation. Very little info there besides saying it didn't work with a default gitlab installation. But my PR above is passing all the functional tests without an issues.

nejch · 2021-12-20T21:32:45Z

Not sure if you really meant this commit d7c7911 which first added that '.' -> '%2E' translation. Very little info there besides saying it didn't work with a default gitlab installation. But my PR above is passing all the functional tests without an issues.

Sorry yes this is what I meant, I just did a quick blame there. 🤦

Ahh interesting: it seems like the underlying issue was fixed in GitLab 13.2 by upgrading the Grape library. Previously GitLab needed %2E encoding for the API it seems, If I understand this MR and the MRs linked correctly: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/36038. So looks like it's being tested for regressions upstream as well 👍

JohnVillalovos · 2021-12-20T21:37:57Z

Ahh interesting: it seems like the underlying issue was fixed in GitLab 13.2 by upgrading the Grape library. Previously GitLab needed %2E encoding for the API it seems, If I understand this MR and the MRs linked correctly: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/36038. So looks like it's being tested for regressions upstream as well 👍

Nice research!!! 👍

Forcing the encoding of '.' to '%2E' causes issues. It also goes against the RFC: https://datatracker.ietf.org/doc/html/rfc3986.html#section-2.3 From the RFC: For consistency, percent-encoded octets in the ranges of ALPHA (%41-%5A and %61-%7A), DIGIT (%30-%39), hyphen (%2D), period (%2E), underscore (%5F), or tilde (%7E) should not be created by URI producers... Closes #1006 Related #1356 Related #1561 BREAKING CHANGE: stop encoding '.' to '%2E'. This could potentially be a breaking change for users who have incorrectly configured GitLab servers which don't handle period '.' characters correctly.

JohnVillalovos · 2021-12-21T16:15:05Z

@dAnjou Would you be willing to rebase this? I think we fixed the issue that caused the functional tests to fail in a PR that got merged today PR #1766

JohnVillalovos · 2021-12-21T16:55:30Z

@dAnjou Would you be willing to rebase this? I think we fixed the issue that caused the functional tests to fail in a PR that got merged today PR #1766

I got impatient 😊So I did it myself.

dAnjou · 2021-12-21T17:53:43Z

Thank you, @JohnVillalovos! Glad the change made it 😊

JohnVillalovos requested changes Jul 31, 2021

View reviewed changes

dAnjou force-pushed the patch-1 branch from ba4e0da to 3c38eb6 Compare August 1, 2021 10:44

dAnjou force-pushed the patch-1 branch 2 times, most recently from ac51f09 to 3910048 Compare August 6, 2021 13:55

dAnjou force-pushed the patch-1 branch from 05557f2 to dafcef1 Compare December 17, 2021 20:40

JohnVillalovos mentioned this pull request Dec 19, 2021

fix: stop encoding '.' to '%2E' #1766

Merged

feat(api): support file format for repository archive

85252e4

JohnVillalovos force-pushed the patch-1 branch from dafcef1 to 85252e4 Compare December 21, 2021 16:54

JohnVillalovos approved these changes Dec 21, 2021

View reviewed changes

JohnVillalovos enabled auto-merge (rebase) December 21, 2021 16:57

JohnVillalovos merged commit 83dcabf into python-gitlab:main Dec 21, 2021

feat(api): support file format for repository archive #1561

feat(api): support file format for repository archive #1561

Uh oh!

Conversation

dAnjou commented Jul 31, 2021

Uh oh!

codecov-commenter commented Jul 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JohnVillalovos left a comment

Choose a reason for hiding this comment

Uh oh!

dAnjou commented Aug 1, 2021

Uh oh!

dAnjou commented Aug 1, 2021

Uh oh!

nejch commented Aug 1, 2021

Uh oh!

JohnVillalovos commented Aug 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dAnjou commented Aug 6, 2021

Uh oh!

nejch commented Aug 22, 2021

Uh oh!

JohnVillalovos commented Dec 17, 2021

Uh oh!

JohnVillalovos commented Dec 18, 2021

Uh oh!

JohnVillalovos commented Dec 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nejch commented Dec 19, 2021

Uh oh!

JohnVillalovos commented Dec 20, 2021

Uh oh!

nejch commented Dec 20, 2021

Uh oh!

JohnVillalovos commented Dec 20, 2021

Uh oh!

JohnVillalovos commented Dec 21, 2021

Uh oh!

JohnVillalovos commented Dec 21, 2021

Uh oh!

dAnjou commented Dec 21, 2021

Uh oh!

Uh oh!

codecov-commenter commented Jul 31, 2021 •

edited

Loading

JohnVillalovos commented Aug 1, 2021 •

edited

Loading

JohnVillalovos commented Dec 18, 2021 •

edited

Loading