- Notifications
You must be signed in to change notification settings - Fork675
fix: optionally keep user-provided base URL for pagination#2149
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-commenter commentedJul 17, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #2149 +/- ##======================================= Coverage 95.46% 95.46% ======================================= Files 81 81 Lines 5353 5362 +9 =======================================+ Hits 5110 5119 +9 Misses 243 243
Flags with carried forward coverage won't be shown.Click here to find out more.
|
JohnVillalovos commentedJul 18, 2022
I wonder if this would be better to be a config option? As probably the most likely case when this occurs is that something is wrong with their config. |
Uh oh!
There was an error while loading.Please reload this page.
iomarmochtar commentedJul 18, 2022
do you mean by set the config as parameter in |
nejch commentedJul 18, 2022
Thanks for the work here@iomarmochtar! Have a look at#1978 as well, which covers mostly the same topic. I agree with John we should at least not completely blindly follow either the server or the client-provided URL when there is a mismatch, hence my proposal in the issue above: I think we should issue a warning instead, unless explicitly configured to follow the base URL, and only then reconstruct the URL. Could we add that here? 🙇 Thanks again! For context: I actually would consider this approach a misconfiguration of the Instead I think they should use |
9f52060 toc815b94Comparenejch commentedJul 29, 2022
@iomarmochtar would you still like to work on this as an optional parameter as discussed above? |
iomarmochtar commentedJul 29, 2022
thanks you for remind me@nejch ,almost forgot to continue it due office task. let me try in this weekend. |
c815b94 toc0f35d4Compareiomarmochtar commentedJul 30, 2022
push as requested, please review it@nejch@JohnVillalovos |
nejch left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks again@iomarmochtar. I have a few small comments
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
c0f35d4 to9c02633Comparenejch commentedAug 3, 2022
Thanks@iomarmochtar. I have some ideas to reuse this elsewhere that I think are outside the scope of this PR, so I'll merge this as is, and I will do a little refactor after. |
Background
To enhance HTTP security our Gitlab is fronted byIAP for example
https://gitlab.tools.domain.iobut for internal communication from runner to Gitlab API we create another endpoint that protected by firewall rules so then it only can be accessed from certain IP only, for examplehttps://runner.gitlab.tools.domain.io/it actually using the reverse proxy that in the upstream destination will setHostheader to the original one (gitlab.tools.domain.io).But when we run a python script inside Gitlab pipeline to the endpoint (
runner.gitlab.tools.domain.io) and pass argumentall=Trueoriterator=Truefor loop all the data to all page will returning the base url asgitlab.tools.domain.iowhereas it will causing the error due the request will be blocked by IAP.Expected
The base url will be persist same as the first time set in the next url so the request will be expected goes as the first page when using pagination iterrator (
all=True).Fix
This MR will make sure the base in next url is the same. It's been tested in my case and working as expected.