-
Notifications
You must be signed in to change notification settings - Fork 80
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
PDF: Switch to using Proton #1090
Conversation
if (req.query.new_pdf) { | ||
return newResult; | ||
} | ||
)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to do more - change the public API to allow for changing the hardcoded /a4/desktop
. Optional parameters with defaults should not break backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, damn, forgot that part. Very good point. Will change.
`${options.new_uri}/${rp.domain}/v1/` | ||
+ `pdf/${encodeURIComponent(rp.title)}/a4/desktop` | ||
)) | ||
.catch((e) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still keep logging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't throwing the error here make RB log it automatically? We don't want to end up with duplicate log entries.
@@ -47,11 +47,7 @@ default_project: &default_project | |||
pdf: | |||
# Cache PDF for 5 minutes since it's not purged | |||
cache_control: s-maxage=600, max-age=600 | |||
uri: https://pdfrender-beta.wmflabs.org | |||
new_uri: https://proton-beta.wmflabs.org |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe let's rename new_uri to uri?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about it, but to keep it backwards compatible, I opted with leaving it as-is. Perhaps we could go with this now and revisit in the future?
4818c45
to
0f3597c
Compare
@Pchelolo updated the URI |
We are sunsetting Electron and switchng fully to Proton. Therefore, adjust the code to use Proton only if `new_uri` is specified (and consequently get rid of `new_probability` and `new_pdf`). However, for the sake of backwards compatibility, keep the code using Electron around. Bug: T210651
0f3597c
to
1835094
Compare
Superseded by #1126 |
We are sunsetting Electron and switchng fully to Proton. Therefore, adjust the code to use Proton only if
new_uri
is specified (and consequently get rid ofnew_probability
andnew_pdf
). However, for the sake of backwards compatibility, keep the code using Electron around.Bug: T210651