- 
                Notifications
    You must be signed in to change notification settings 
- Fork 14
          feat: Add KeyValueStoreClient(Async).get_record_public_url
          #506
        
          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
Conversation
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.
The changes seem sound to me, thank you! 👍
I have one nit / idea regarding the tests:
| expected_signature = f'?signature={public_url.split("signature=")[1]}' if signing_key else '' | ||
| assert public_url == ( | ||
| f'{(api_public_url or DEFAULT_API_URL).strip("/")}/v2/key-value-stores/someID/keys{expected_signature}' | ||
| ) | 
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.
In all the (new) tests, it seems we're parsing the signature query param from the public_url, only to test if public_url contains this query param.
something like
const { signature } = public_url;
assert public_url === `abc.def/${signature}`;
Can we check whether, e.g., the signature is the actual HMAC of the key instead?
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.
Added expected signature value to the tests
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.
LGTM
a6f448a    to
    e1bfec3      
    Compare
  
    ### 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
KeyValueStoreClient.get_record_public_url.KeyValueStoreClientAsync.get_record_public_url.Issues
Closes: #497