Codecov Report

Merging#1561 (3910048) intomaster (ae97196) willdecrease coverage by4.49%.
The diff coverage is33.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

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

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 fromhttps://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 fromhttps://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 intotest_repository_archive() and try adding a different format:

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 fromhttps://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 thenote

As of 2021-08, the format options are: tar.gz, tar.bz2, tbz, tbz2, tb2, bz2, tar, and zip.For the formats available, refer tohttps://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 fortar.gz andtar.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 fortar.gz andtar.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 fortar.gz andtar.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 itis 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:

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

Ok, so itis 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 whata221d7b 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 ind56a434 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 commitd7c7911 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 commitd7c7911 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!!! 👍

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