Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
doc: change smartos support type to experimental#56220
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Uh oh!
There was an error while loading. Please reload this page.
fc1cf65 to 9e99d90Comparebahamat commented Dec 11, 2024
I've replied to nodejs/TSC#1663. Given my comments there, I think it's premature (at best) to demote SmartOS. I vote to close this PR without merging. |
bahamat left a comment
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.
Further discussion needed as per nodejs/TSC#1663.
marco-ippolito commented Dec 18, 2024
Are there other cases of tier 2 platforms that we don't release binaries for? |
mcollina left a comment
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.
Considering that is now working properly, I think it’s ok.
anonrig commented Dec 18, 2024
I disagree. |
joyeecheung commented Dec 18, 2024
I opened nodejs/TSC#1666 as an alternative - IMO, we should change how we treat tier 2 platforms (i.e. don't insist on keeping all tests green in every PR on them) to make it sustainable. Otherwise there's effectively no difference between tier 1 and tier 2, and basically only tier 1 and experimental, which would lead to this PR. |
anonrig commented Jan 8, 2025
I agree with @joyeecheung's comment on this issue, and believe we should proceed with this pull-request: nodejs/TSC#1666 (comment)
For visibility: @nodejs/tsc |
mcollina commented Jan 8, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
100% of the maintainers of @nodejs/platform-smartos have asked that PRs are runs against those machines. |
bahamat commented Jan 8, 2025
Yes, that's correct. I believe this was also something that the RH/IBM/AIX folks wanted, so SmartOS/illumos is not alone here. The new SmartOS build servers have been running smoothly since mid-December. During that time there was one issue raised to us that was resolved pretty much right away. |
jasnell commented Jan 8, 2025
Running all PRs against those machines is fine. I think the question is whether failures on those machines should be considered blocking in order for the PR to land, especially if the reason those fail is due to the CI image or compiler environment being out of date or, at least, not up to the minimum requirements. Right now, if the smartos CI runners are updated and working correctly then I would say that the current issue is resolved, at least for now. But longer term we should address the larger question here given how disruptive these kinds of issues have been to landing PRs. We've had PRs blocked for months due to CI issues like this and I don't want to get back into that kind of state. If there are still issues with the smartos runners blocking PRs right now, let's list those issues and get them dealt with now. |
bahamat commented Jan 9, 2025
I wholeheartedly agree with this statement. To my knowledge there are zero PRs blocked by SmartOS. If I am mistaken in this, I would like to know so that we can address it as soon as possible. |
jasnell left a comment
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.
For now, let's leave SmartOS at Tier 2.
anonrig commented Jan 22, 2025
Removing from tsc-agenda. I've been asked to drop this PR. Ref: https://github.com/nodejs/TSC/pull/1679/files#diff-dbed981138b822baa9f261923bfd02ea9ce949ba83166aade23c9e7cc46c65beR71 |
Follow up for nodejs/TSC#1663
cc @nodejs/build @nodejs/tsc @nodejs/platform-smartos