Skip to content
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

Fixed an issue where the mapper already has https:// address assigned… #42

Merged
merged 2 commits into from
May 20, 2024

Conversation

turetske
Copy link
Collaborator

… during creation but the url parsing was failing to catch it

Added a test of the get_mapper to the code

… during creation but the url parsing was failing to catch it

Added a test of the get_mapper to the code
@turetske turetske requested a review from bbockelm May 14, 2024 15:52
@turetske turetske mentioned this pull request May 19, 2024
@turetske
Copy link
Collaborator Author

This is meant to be a temporary fix. There is an issue which discusses a more long term solution: #43

Copy link
Contributor

@bbockelm bbockelm left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for dropping in a unit test (and a comment) so it's obvious to future readers why the non-pelican:// check exists.

Note this won't work with http:// URLs (which can still exist for some caches, no?).

@turetske
Copy link
Collaborator Author

LGTM. Thank you for dropping in a unit test (and a comment) so it's obvious to future readers why the non-pelican:// check exists.

Note this won't work with http:// URLs (which can still exist for some caches, no?).

Good point, I'll add that as a check as well.

@turetske turetske merged commit 17ada18 into PelicanPlatform:main May 20, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants