Force https for urls in scraper #2222
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
OpenAPI ChangesShow/hide No detectable change. |
There was a problem hiding this comment.
I'm getting 70853 on both main and your branch after restarting all containers when running
from learning_resources.site_scrapers.utils import scraper_for_site
scraper = scraper_for_site("http://micromasters.mit.edu/scm/")
content = scraper.scrape()
print(len(content))But this change makes sense to me, and seems plausible that it's not always reproducible, so 👍
Edit: Making sure EMBEDDINGS_EXTERNAL_FETCH_USE_WEBDRIVER=True, now I get expected difference. 👍
| @@ -5,6 +5,7 @@ | |||
|
|
|||
|
|
|||
| def scraper_for_site(url): | |||
| url = url.replace("http://", "https://") | |||
There was a problem hiding this comment.
BTW http:// is legal in query parameters, so theoretically this could change more than the protocol. (E.g., our login URL is https://api.rc.learn.mit.edu/learn/login?next=https://rc.learn.mit.edu/)
Very likely those sorts of URLs don't matter for this scraping (and maybe changing http to https in query params would be desirable, too).
OpenAPI ChangesShow/hide No detectable change. |
OpenAPI ChangesShow/hide No detectable change. |
OpenAPI ChangesShow/hide No detectable change. |
ChristopherChudzicki
approved these changes
May 1, 2025
This was referenced May 6, 2025
Closed
Closed
Closed
Closed
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/7216
Description (What does it do?)
This PR makes it so that http start urls are forced to https before beginning the scraping process. Some of the apis our etls hit return http urls and the redirect to https is causing unexpected behavior.
How can this be tested?