Skip to content

feat: Added get_public_url method to KeyValueStore#572

Merged
vdusek merged 35 commits intoapify:masterfrom
akshay11298:kvstore-public-url
Oct 22, 2024
Merged

feat: Added get_public_url method to KeyValueStore#572
vdusek merged 35 commits intoapify:masterfrom
akshay11298:kvstore-public-url

Conversation

@akshay11298
Copy link
Copy Markdown
Contributor

@akshay11298 akshay11298 commented Oct 6, 2024

Description

  • Implement get_public_url method in KeyValueStore

Issues

Testing

  • Unit tests added

Checklist

  • CI passed

@janbuchar
Copy link
Copy Markdown
Collaborator

This looks like the right approach. You should use the storage_dir attribute of the configuration object instead of the current directory though.

@akshay11298
Copy link
Copy Markdown
Contributor Author

This looks like the right approach. You should use the storage_dir attribute of the configuration object instead of the current directory though.

@janbuchar Thanks for the review and hint :) I have updated the PR with storage_dir

@fnesveda fnesveda added the t-tooling Issues with this label are in the ownership of the tooling team. label Oct 9, 2024
@akshay11298 akshay11298 requested a review from janbuchar October 10, 2024 02:19
@janbuchar
Copy link
Copy Markdown
Collaborator

@akshay11298 sorry for the broken CI. I confirm that code quality checks are passing on my machine.

@akshay11298 akshay11298 requested a review from janbuchar October 10, 2024 12:11
@akshay11298 akshay11298 requested a review from janbuchar October 10, 2024 13:04
@akshay11298
Copy link
Copy Markdown
Contributor Author

akshay11298 commented Oct 20, 2024

@janbuchar @vdusek found the cause of the error. It was the slash used when creating the path manually. Used os.path.join to fix the error

Edit:
Just saw the build result, the test is not able to read the file. Its giving permission denied error. Although I am wondering if this was created by the application, ideally it should have the access as well. I will be checking this. Although any idea on why it may occur though?

Akshay Avinash added 2 commits October 22, 2024 06:25
@akshay11298
Copy link
Copy Markdown
Contributor Author

akshay11298 commented Oct 22, 2024

@janbuchar @vdusek Now the test passes. urlparse was returning netloc in windows and path in mac/linux

Windows -
image

Mac -
image

Copy link
Copy Markdown
Collaborator

@vdusek vdusek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @akshay11298, for solving it. I added one more test, test_get_public_url_raises_for_non_existing_key, and updated the method a bit so that it passes. LGTM.

@vdusek vdusek requested a review from janbuchar October 22, 2024 07:20
Copy link
Copy Markdown
Collaborator

@janbuchar janbuchar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Collaborator

@janbuchar janbuchar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vdusek vdusek merged commit 3a4ba8f into apify:master Oct 22, 2024
@akshay11298 akshay11298 deleted the kvstore-public-url branch October 22, 2024 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-tooling Issues with this label are in the ownership of the tooling team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement KeyValueStore.get_public_url

4 participants