fix: Presigned resource urls shouldn't follow base url #500
Conversation
github-actions
bot
added
t-tooling
barjin
left a comment
barjin
left a comment
There was a problem hiding this comment.
Thank you!
Looks mostly good to me, although I'd prefer to leave the approval to an actual Python dev :) Some Grammarly nits about the comments and one question:
Co-authored-by: Jindřich Bär <jindrichbar@gmail.com>
barjin
left a comment
barjin
left a comment
There was a problem hiding this comment.
Looks good to me, thank you for the fixes!
| assert kvs_client._url() == f'{non_existent_url}/v2/key-value-stores/{kvs_name}' | ||
| assert kvs_client._url(public=True) == f'{DEFAULT_API_URL}/v2/key-value-stores/{kvs_name}' |
There was a problem hiding this comment.
nit: should we test _-prefixed methods?
There was a problem hiding this comment.
Fair point, I wanted to avoid mocking the API calls, but that was actually the correct approach. I reworked the tests.
tests/integration/conftest.py
Outdated
| ('https://api.apify.com', 'https://custom-public-url.com'), | ||
| ('https://api.apify.com', 'https://custom-public-url.com/with/custom/path'), | ||
| ('https://api.apify.com', 'https://custom-public-url.com/with/custom/path/'), | ||
| ('http://10.0.88.214:8010', 'https://api.apify.com'), |
There was a problem hiding this comment.
I hate to be a stickler for detail, but now we're testing this:
client = ApifyClient(base_url="10.0.0...", public_base_url="https://api.apify.com")
assert "api.apify.com" in client.kvs.public_url() While the failing case from the original issue would be something like
client = ApifyClient(base_url="10.0.0...") # no public url passed, only base
assert "api.apify.com" in client.kvs.public_url() # yet, this assert still passesNot that it really matters, but I wanted to propose this with the previous commit and ultimately revoked my comment, because I noticed this difference :)
janbuchar
left a comment
janbuchar
left a comment
There was a problem hiding this comment.
If I'm not mistaken, there will need to be a change in the SDK as well so that it passes the correct public api url on the platform?
### Description - Use the Apify client for creating a public URL. This was added in the latest release of the client: - apify/apify-client-python#500 - apify/apify-client-python#506 - Refactor Apify storage creation to reduce code duplication ### Issues - Closes: #612 - Related: #635
Description
baseUrlapify-client-js#745Issues
Testing