coverage: add the needed CSS from the main site - #1947
Conversation
|
Can someone help me out with the above 2 TODO? Mostly the python script change to include the new styles.css. |
|
I still think we should also move the other assets like the logo and favicons so that this is completely independent from the main site. Edit: we already changed this and we use the assets directly. Regardless this should be better, everything will be isolated. |
|
@rvagg maybe you can give me a hand here? |
|
python aint' my thing, the original author isn't around anymore, and we've lost a bunch of Python folks, so I'm not sure who else to call on other than @cclauss. |
|
Can we get some help here please? |
Which Python script? |
|
Something like: with open('jenkins/scripts/coverage/styles.css') as in_file:
with open('out/styles.css') as out_file:
out_file.write(in_file.read()) |
|
Can you push to this branch? People with push rights can push to a fork's PR branch. I cannot try it nor confirm the fix, hence why I suggest someone else to do it. |
|
That code seem like the desired functionality? I am on an iPhone so I will not push for several hours. |
|
There's no hurry :) I just want someone to test this beforehand, and also take the responsibility of the change :P |
|
The similar change for benchmarking broken things because the SCP copies need to be updated as well. Just an FYI that before this lands we should double check if a similar change is needed. . |
|
So, any news? Anyone who can help with the python script? |
|
@nodejs/coverage-admins @nodejs/testing we seem to have a void in our working knowledge of how coverage.nodejs.org is built and updated. benchmarking.nodejs.org is similar but gets more attention. This PR appears to be touching code that was originally authored by folks who are no longer with us or whose attention is elsewhere. Perhaps we need to do some more radical surgery here and replace what's being done here with code that more people understand and can maintain? (IMO the choice of Python for our tooling should be a last resort, we don't have many active folks who are handy enough with Python). Is coverage.nodejs.org even something that should be actively maintained as it is or are we migrating to this new third-party hosted tool? |
I think we need to keep it for a while, although it would be nice to eventually rely on codecov.io instead, I think. @bcoe |
|
Sorry. I will complete this one. I do agree that in general we should keep converting Python code to JS code. |
|
Should I move the rest of the assets like favicons to the repo too? |
|
@rvagg @Trott I think we can consider migrating to codecov.io sometime in the new year, but there are a few bugs on their end that I'm working with them to sort out:
CC: @thomasrockhu, do you think you'll have any cycles in the new year to address the block coverage issues? |
|
This is blocking other work (particularly nodejs/nodejs.org#2635) so I'm adding to the meeting agenda in the hopes that we can move this forward. |
This unties the site from the upstream CSS
|
Do note that the current live site on https://coverage.nodejs.org has its footer broken already. |
|
I don't want to maintain it :P This is just the proper way moving forward so that child sites do not use the upstream CSS thus avoiding breakage. This patch just needs a few lines in the python script to copy the newly added styles.css file. Personally, I'd even add all the assets like the favicons too, but that's for another day. |
|
So, I've never personally looked at the coverage site (until an hour ago). I've no idea who maintains it, or how it works, what a "child site" is, what the "upstream CSS" is, or what would happen if this PR merged (if anything, maybe some kind of follow steps are needed?). I'm sorry, I'm sure that is not so satisfying, but basically, you'll need to find someone who is involved with maintaining that site to review this. Do you know who the right person to bug is? If the site isn't maintained by anybody, it unfortunately might need to be removed, or just left as-is if it offers some value. |
|
I don't understand why "I" have to do all this? I simply submitted a patch to fix something that's known to break. I don't have the time nor the knowledge to do something more. |
|
Weird, if you have push rights to the build repo you should be able to push
to the pull request branch. I can try tomorrow although the path seems off
judging by the other code (Jenkins isn't used in the other places)
…On Tue, Jan 28, 2020, 21:12 Christian Clauss ***@***.***> wrote:
@XhmikosR <https://github.com/XhmikosR> I can not push to your fork but
if you can please paste this code
<#1947 (comment)> *just
above line 6* of the Python file? We can then see if that achieves your
objective.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1947?email_source=notifications&email_token=AACVLNMSEX4AE74EJZ2AL43RAB7SLA5CNFSM4I675I62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKEQ7KY#issuecomment-579407787>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACVLNILMLUFWGQROBW5OT3RAB7SLANCNFSM4I675I6Q>
.
|
|
I am not in @nodejs/build but I believe that you and I can work this together. |
|
@cclauss done, can you test it? |
|
It is not clear to me how to test this because Ideas from the others on how to test this change? |
|
this thread keeps on making it clear that there is no owner for this stuff. It's probably time to remove all of the scripting, and probably styling (directory listings are perfectly usable), from it, anything that carries a maintenance burden that there's nobody to shoulder. |
|
Maybe @mhdawson knows more? Either way, this patch is a good change for as you can see the current live site has its footer already broken. |
|
I don't really think we need to go deleting things. The main reason I've not landed this is that nothing is currently broken, the PR is a suggested improvement. When I land I want to have time to validate things are ok. That has just not lined up in terms of having to do it at a good time, and since nothing is actually broken I've not made it a priority. |
|
Landing this may just work, but will have to check first it the scripts copy everything from the directory of specific files. To be honest given that the end goal is to migrate to codecov.io I'd prefer to just leave things as are as opposed to having to spend time to update and possibly investigate if things are broken. |
|
I'll also add that I've not been completely ignoring the coverage site as I recently did a cleanup after it was reported that we were not getting data... |
|
What was #1947 (comment) all about?? |
|
@cclauss that is newish info that I'd not picked up. |
|
I just checked and the full out directory should be copied over. That means that this should likely be ok. Given that I suggest we just land and then check its ok (I think it runs 4 times a day from the cron job). Not a great time since tomorrow is my last day before heading off on vacation but I assume somebody else can back it out if there is a problem and I can't get to it. |
|
Coverage run so that we find out if its ok sooner than later: https://ci.nodejs.org/view/Node.js%20Daily/job/node-test-commit-linux-coverage-daily/415/ |
|
The copy failed unfortunately. Going to move copy of styles file to the ci job instead. |
|
New CI do see if its ok now: https://ci.nodejs.org/view/Node.js%20Daily/job/node-test-commit-linux-coverage-daily/ |
|
One more time after tweaking job to get the copy to the right directory: https://ci.nodejs.org/view/Node.js%20Daily/job/node-test-commit-linux-coverage-daily/417/ |
|
All good now. |
7 participants
This unties the site from the upstream CSS
I need some help actually copying the CSS file. Any help welcome.
Preview: https://relaxed-ptolemy-556131.netlify.com/
Refs nodejs/nodejs.org#2635
TODO:
Note that the CSS is only the required. Which means that if more elements are added at some point, their rules should be added manually here again.