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

Explicit cloudflareClient if Cloudflare hosted #6613

Merged
merged 2 commits into from
Dec 18, 2024

Conversation

vetleledaal
Copy link
Contributor

Inspired by #6609, change necessitated by J2K not setting client to network.cloudflareClient.

Many moving pieces:

  • All extensions using super.client explicitly were checked
  • If baseUrl was hosted on Cloudflare it was changed
  • 180/187† were multisrc and version was not bumped for these (for this code change)
  • If the multisrc did not have client set‡ to network.cloudflareClient it was set and bumped
  • This will bump 64 multisrc extensions and 6 individual extensions

† estimate
‡ blogtruyen, mangahub, mmrcms, pizzareader, zeistmanga

Yes, I am aware that changing super.client to network.cloudflareClient has no effect for multisrc extensions. The scope was limited to only target Cloudflare hosted services. Another angle may be revisited in another PR.


Has not been tested exhaustively since the code change itself is trivial. Relying on CI for lint issues. Reviewing this PR is probably difficult. Here are some notes I used, I'm not sure if it tells you (the reviewer) anything.

Themes modified and what client is set to (excluding mangahub because client was set in multisrc):

themePkg = 'blogtruyen'       unset
themePkg = 'fmreader'         cloudflare
themePkg = 'fuzzydoodle'      cloudflare
themePkg = 'gattsu'           cloudflare
themePkg = 'goda'             cloudflare
themePkg = 'heancms'          cloudflare
themePkg = 'keyoapp'          cloudflare
themePkg = 'liliana'          cloudflare
themePkg = 'madara'           cloudflare
themePkg = 'mangathemesia'    cloudflare
themePkg = 'manhwaz'          cloudflare
themePkg = 'mmrcms'           unset
themePkg = 'peachscan'        cloudflare
themePkg = 'pizzareader'      unset
themePkg = 'terrascan'        cloudflare
themePkg = 'wpcomics'         cloudflare
themePkg = 'zeistmanga'       unset
themePkg = 'zmanga'           cloudflare

Cloudflare sites were checked with:

rg --no-filename -C9999 "client(: OkHttpClient)? = super\.client" | grep -Eo 'https://[^"/]+' | grep -vF '$' | sort -u | xargs -I {} sh -c 'curl -sI --connect-timeout 10 --max-time 10 "{}" | grep -i "server:" | grep -qi "cloudflare" && echo "{}"'

Checklist:

  • Updated extVersionCode value in build.gradle for individual extensions
  • Updated overrideVersionCode or baseVersionCode as needed for all multisrc extensions
  • Referenced all related issues in the PR body (e.g. "Closes #xyz")
  • Added the isNsfw = true flag in build.gradle when appropriate
  • Have not changed source names
  • Have explicitly kept the id if a source's name or language were changed
  • Have tested the modifications by compiling and running the extension through Android Studio
  • Have removed web_hi_res_512.png when adding a new extension

@bapeey bapeey mentioned this pull request Dec 14, 2024
8 tasks
Copy link
Contributor

@AwkwardPeak7 AwkwardPeak7 left a comment

Choose a reason for hiding this comment

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

most multisrc source here already use cloduflareclient in the multisrc so I don't think it should be changed here

@vetleledaal
Copy link
Contributor Author

I have conditionally reverted the changes done in ./src/ for multisrc extensions. The overall diff should be smaller.

rg 'overrideVersionCode =' | cut -d: -f1 | cut -d/ -f1-3 | xargs git restore --source=HEAD~

@AwkwardPeak7 AwkwardPeak7 merged commit 68b3952 into keiyoushi:main Dec 18, 2024
23 checks passed
@cuong-tran
Copy link
Contributor

How about cases where extensions are using network.client directly, which is same as super.client I think?

@vetleledaal
Copy link
Contributor Author

Use of network.client would use the non-cloudflareClient in J2K. Since it is less commonly used than super.client I simply forgot about that usage pattern.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants