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

Replace http->https in URLs in DependencyMetadata for some Forges #3583

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

danicheg
Copy link
Contributor

@danicheg danicheg commented Feb 9, 2025

Hey there. Sorry for not starting with some discussion somewhere in Discord, hopefully, this PR is also fine to discuss the idea.

Motivation

We use org.scalasteward.core.util.UrlChecker to validate URLs in several places, such as in org.scalasteward.core.nurture.UpdateInfoUrlFinder. In turn, some projects may have HomePage and ScmUrl for github.com, gitlab.com and bitbucket.org with the http scheme. Given how we check for URL existence, the check easily returns false, even though all the mentioned Forges forward to the https scheme.

❯ curl -I http://github.com/lettuce-io/lettuce-core
HTTP/1.1 301 Moved Permanently
Location: https://github.com/lettuce-io/lettuce-core

❯ curl -I http://bitbucket.org/krake-oss/wurbelizer/
HTTP/1.1 301 Moved Permanently
location: https://bitbucket.org/krake-oss/wurbelizer/

❯ curl -I http://gitlab.com/fdroid/fdroidclient
HTTP/1.1 301 Moved Permanently
Location: https://gitlab.com/fdroid/fdroidclient

Ultimately, I think having this strict check might be reasonable (since we have different usage of it across the codebase), as well as automatically substituting http with https when instantiating org.scalasteward.core.coursier.DependencyMetadata.

I’d appreciate your thoughts on this.

Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.86%. Comparing base (231fe8f) to head (5d46d7b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3583      +/-   ##
==========================================
+ Coverage   89.80%   89.86%   +0.06%     
==========================================
  Files         174      174              
  Lines        5031     5061      +30     
  Branches      493      488       -5     
==========================================
+ Hits         4518     4548      +30     
  Misses        513      513              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fthomas
Copy link
Member

fthomas commented Feb 10, 2025

I’d appreciate your thoughts on this.

If Scala Steward shows/uses more URLs, that is a good thing because those links in PR descriptions are really valuable. 👍 But I'm unsure about tying this to the forges Scala Steward supports. I could imagine there being other sites where we would like to replace http with https but then can't because it is not a ForgeType in Scala Steward. I'd therefore prefer a hard-coded list of hosts where schemes are replaced instead of modifying ForgeType.

@danicheg
Copy link
Contributor Author

I'd therefore prefer a hard-coded list of hosts where schemes are replaced instead of modifying ForgeType.

Heh, that was exactly what I was trying to bypass. When I found ForgeType, it seemed like the perfect place since we already have a data type modeling the domain. It already defines publicWebHost, so adding publicWebScheme nearby doesn’t make things worse imho. We could even introduce something like publicWebEndpoint to encapsulate both the host and scheme in a single entity — but maybe that’s overkill.

What about other sites, it shouldn't be that difficult to add specific support as a predefined Forge for them. If we speak about arbitrary sites over the Web... I don't think this is the case we should wonder about tbh.

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