nodejs / nodejs.org

The Node.js® Website
https://nodejs.org
MIT License
6.15k stars 6.21k forks source link

fix: ci #6774

Closed AugustinMauroy closed 4 months ago

AugustinMauroy commented 4 months ago

Description

This key in json was introduce during a past pr. It's introduce by "old" turborepo version. Basically, if we were using node 22, it would call corepack, which would install pnpm (What I understood from my experiments)

Validation

CI need to be green again

Check List

vercel[bot] commented 4 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Jun 2, 2024 7:57pm
github-actions[bot] commented 4 months ago
Lighthouse Results URL Performance Accessibility Best Practices SEO Report
/en 🟠 83 🟢 100 🟢 96 🟢 91 🔗
/en/about 🟢 98 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 99 🟢 100 🟢 100 🟢 92 🔗
/en/download 🟢 100 🟢 100 🟢 96 🟢 91 🔗
/en/blog 🟢 99 🟢 100 🟢 96 🟢 92 🔗
github-actions[bot] commented 4 months ago

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 92%
90.67% (593/654) 76.08% (175/230) 94.57% (122/129)

Unit Test Report

Tests Skipped Failures Errors Time
131 0 :zzz: 0 :x: 0 :fire: 5.939s :stopwatch:
bmuenzenmeyer commented 4 months ago

Which commit added this line?

https://github.com/nodejs/nodejs.org/commit/d09e4f1a542a80fc63b78b9f245f6382144e540b

Good example to focus on atomic commits in our feature development.

bmuenzenmeyer commented 4 months ago

@AugustinMauroy responding to your confused emoji, I'll share this link: How atomic Git commits dramatically increased my productivity - and will increase yours too

Each commit does one, and only one simple thing, that can be summed up in a simple sentence.

In our case, commit might not be the operative word. Change, or PR would be. The feature where this was introduced didnt have much to do with pinning pnpm - and in fact we don't use pnpm, so it should have been omitted.

And let me be clear, I'm not blaming you for any of this. We work together as a team. We share our wins and our shortcomings. The change went through numerous reviews, and I merged it. This comment was only left in the interest of educating, if you wanted to learn more about the concept.


And to go even further, I've really appreciated your drive to learn, your attention to detail, and your increased skill here on the project. That you identified this problem and worked to fix it should be celebrated.

AugustinMauroy commented 4 months ago

@bmuenzenmeyer sorry for the confusion the emoji meant suprised/confused

And thanks for the links it's very interesting

ovflowd commented 4 months ago

this is a hotfix, merging it to unblock our ci