Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

remove macOS for most projects #971

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

UlisesGascon
Copy link
Member

@UlisesGascon UlisesGascon commented Sep 10, 2023

Refs: #959 (comment)

Checklist
  • npm test passes
  • tests are included
  • documentation is changed or added
  • contribution guidelines followed
    here

@UlisesGascon UlisesGascon changed the title chore: remove macOS for most projects remove macOS for most projects Sep 10, 2023
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a temporary change, right?

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine as a temp change

@UlisesGascon UlisesGascon marked this pull request as ready for review September 14, 2023 16:31
@UlisesGascon
Copy link
Member Author

It's a temporary change, right?

Yes, the idea is to restore them one by one over the time

@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (2baac2e) 96.44% compared to head (1fc650b) 96.44%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #971   +/-   ##
=======================================
  Coverage   96.44%   96.44%           
=======================================
  Files          28       28           
  Lines        2139     2139           
=======================================
  Hits         2063     2063           
  Misses         76       76           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@UlisesGascon
Copy link
Member Author

CI is green now 🎉

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Trott
Copy link
Member

Trott commented Sep 21, 2023

Did the removal of -J make this superfluous? If not, let's land it.

@Trott
Copy link
Member

Trott commented Sep 21, 2023

macOS hasn't run on any of the last 4 or 5 CITGM runs in Jenkins. Is that something intentional that someone did or is that a strange anomaly to be looked at?

@Trott
Copy link
Member

Trott commented Sep 21, 2023

(It last ran as part of https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3252/ but we're up to https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3257/ now and they all completed without a macOS job.)

@richardlau
Copy link
Member

macOS hasn't run on any of the last 4 or 5 CITGM runs in Jenkins. Is that something intentional that someone did or is that a strange anomaly to be looked at?

Ah that's probably nodejs/build#3452 -- we no longer run on macOS 10.15 for Node.js 21 (i.e. main) and haven't added a newer macOS to the matrix for the CITGM job -- @UlisesGascon do you think we have the space on the macOS 11 VMs to add an additional Jenkins workspace (for the CITGM job)?

@UlisesGascon
Copy link
Member Author

Ah that's probably nodejs/build#3452 -- we no longer run on macOS 10.15 for Node.js 21 (i.e. main) and haven't added a newer macOS to the matrix for the CITGM job -- @UlisesGascon do you think we have the space on the macOS 11 VMs to add an additional Jenkins workspace (for the CITGM job)?

Yes, I think we can use one slot for it in Orka, based on this comment we can use macpro-4 to host it.

@richardlau
Copy link
Member

@UlisesGascon I was referring to extra disk space usage (for the citgm-smoker workspace), not additional VMs.

@Trott Trott mentioned this pull request Oct 2, 2023
@Trott
Copy link
Member

Trott commented Oct 2, 2023

Based on #997, maybe it's time to land this? @UlisesGascon If you want to rebase this, please do! (Otherwise, I'll do it at some point and force-push it to your branch. Either way!)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants