-
Notifications
You must be signed in to change notification settings - Fork 70
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
Remove all the backend usage of plugin site API #961
Conversation
6d54a9c
to
781724e
Compare
whoops, jenkinsfile isn't updated to node 16, tomorrow task I guess |
].map(async type => { | ||
try { | ||
let url = wiki.url.replace('https://github.com/', 'https://raw.githubusercontent.com/'); | ||
if (!url.includes('/tree/')) { |
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.
This if
may need an else
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.
can you give me an example of why?
the only one that triggers "has no content that can be fetched from" that is github related that I can see is "error DotCi-Fig-template has no content that can be fetched from http://github.com/DotCi/DotCi-Fig-templat"
I nudged ulli on slack asking if he knew how to setup a warnings-ng report for these lines.
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.
btw, anything i saw with tree/ in it, already had the branch/ref
☎ ~ % curl -qLs 'https://updates.jenkins.io/current/plugin-documentation-urls.json' | jq '.' | grep tree -B 1
},
"absint-astree": {
--
"cloudify": {
"url": "https://github.com/jenkinsci/cloudify-plugin/tree/cloudify-1.0.8"
--
"ibm-application-security": {
"url": "https://github.com/jenkinsci/appscan-plugin/tree/deprecatedPlugin"
--
"kryptowire": {
"url": "https://github.com/jenkinsci/kryptowire-plugin/tree/master/docs"
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 didn't test it locally, but I think the string tree/
should be removed from the URL. The log you linked says it's downloading from https://raw.githubusercontent.com/jenkinsci/cloudify-plugin/tree/cloudify-1.0.8/README.md , should be just https://raw.githubusercontent.com/jenkinsci/cloudify-plugin/cloudify-1.0.8/README.md (failed fetch is currently not logged, AFAIU).
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 good catch!
failed fetch is currently not logged, AFAIU).
failed as in 404? I specifically disabled that because 3/4 of those URLs would be 404 and that's a lot of noise. I'll add that plugin to my specific tests and try again.
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 only log something if all 4 attempts end up as 404? (Non-blocking either way.)
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.
thats what I tried to do, I should abstract it into a function so I can test it more.
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.
fixed, but i havn't had time to fully locally test it (been busy helping infra stabilize), so gonna see what CI does for a full build
797933f
to
cc1b67a
Compare
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.
All images in git plugin docs disappeared because of a 400 http status (wrong base folder for resolving image filenames). Also IIRC we can't load SVG images via raw.githubusercontent, we have to keep using https://cdn.jsdelivr.net/ (maybe for all images).
So not all, went to look at one of the disappearing images on the git plugin looks like github is more forgiving about bad URLs than we are. I'll think about a solution, could just ignore the fact that its a absolute URL and force it to be relative, but also kinda okay with it breaking due to bad data. |
58a47e2
to
83adfd2
Compare
* Removed the root .eslintrc which was breaking things * Still using api for releases (though have a couple ideas) * Try to render README.md, readme.md, README.adoc, readme.adoc, whichever comes back first
702c114
to
c5893aa
Compare
I honestly don't remember what was left undone. Everything seems to work though. I remember we had some absolute vs relative path issues for https://deploy-preview-961--jenkins-plugin-site-pr.netlify.app/git/ but everything seems good. If this gets merged, we can then kill off plugin-site-api, and move everything to netlify and not need to do the azure deploy stuff |
@zbynek any chance you could review this? |
@halkeye the absolute image URL problem is still there: for SVGs I get no error in console, but they seem to be simply missing from DOM:
In general it seems a bit wasteful to try all the readme URLs for all plugins each time the site is rebuilt (for CI builds that run more than once per hour it may run into the 5000 requests/hour limit mentioned in github/docs#8031 (comment) ), but I can't think of a better solution. |
we could call the API to get the readme data directly, but i don't think its better, and would add requireing github tokens to the build. I'll fix up the images. I couldn't find new broken stuff, but I'll admit i didn't spend a lot of time digging into it. |
this is going to be interesting. I don't think we want to blanket support html. Lemme see how I can make it safe parsing html. |
whichever comes back first