-
Notifications
You must be signed in to change notification settings - Fork 7
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
url quote collection info #59
Conversation
Closes #59 |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #59 +/- ##
==========================================
+ Coverage 56.59% 56.75% +0.15%
==========================================
Files 26 26
Lines 947 948 +1
==========================================
+ Hits 536 538 +2
+ Misses 411 410 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
ah ha! Great find. The only other thing I want to look into is more of a curiosity: why I am not seeing the issue in my own environment? I wonder if it's because of my reverse proxy configuration or something else. Going to take a look at that before merging just in case I find something that might effect this. Also it could help me come up with tests that would've caught this. Thanks very much for putting this up! |
Yeah that is odd, it could be the reverse proxy. I'm using nginx 1.20 with Artifactory 7.55. |
@jcox10 @mamercad ok! So I think I figured out what's going on in my environment: due to a different issue, I have used the fallback property option described here: https://github.com/briantist/galactory/blob/main/CHANGELOG.rst#v0-7-0 Since this option sets properties in a The good news is that means there's an existing option you can both use to work around the issue. The bad news is that this thickens the plot: this issue should be addressed in the artifactory library first, and I've opened this issue for it (please upvote 👍): So this PR as written currently would break the fallback mechanism, because it will add encodings to the properties on POST which will end up as literals. But even if we change it to only encode on non-fallback, we'll have the same problem if/when the artifactory library decides to do the same thing. So I want to get an answer from them first on whether they expect clients to encode their own matrix parameter values, or whether they will make/accept a change to do that encoding. Their answer will change how we implement it here. If there's no quick definitive answer or resolution, we may implement it here anyway, possible behind yet another option, or maybe with some other guard in place. In the meantime, could you both check to see if using the property fallback works for you? If there's some issue with it I haven't realized that prevents it from working for you, I'd like to know that too. |
That's a good point, this should be fixed the the artifactory library since it is making the http call. Using the fallback property works for me. |
Use
urllib.parse.quote
to encode any special characters in the collection info. Prevents any query string parameters in the collection info from getting interpreted by Artifactory.Fixes: #58
Fixes: #52
Closes: #54