feat(api): support file format for repository archive by dAnjou · Pull Request #1561 · python-gitlab/python-gitlab · GitHub

No description provided.

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

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.

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.

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.

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

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.

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.

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.

Thanks @dAnjou

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

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?

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.

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.

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.

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 👍

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.

@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

@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.

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 by dAnjou · Pull Request #1561 · python-gitlab/python-gitlab · GitHub

Repository navigation

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

feat(api): support file format for repository archive#1561JohnVillalovos merged 1 commit intopython-gitlab:mainpython-gitlab/python-gitlab:mainfrom dAnjou:patch-1dAnjou/python-gitlab:patch-1Copy head branch name to clipboard

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feat(api): support file format for repository archive#1561
JohnVillalovos merged 1 commit into
python-gitlab:mainfrom
dAnjou:patch-1

codecov-commenter commented Jul 31, 2021 •
edited
Loading

JohnVillalovos commented Aug 1, 2021 •
edited
Loading

JohnVillalovos commented Dec 18, 2021 •
edited
Loading