Make WordPress Core

Opened 15 months ago

Closed 10 months ago

Last modified 10 months ago

#57865 closed task (blessed) (fixed)

GitHub Actions updates and improvements for 6.3

Reported by: desrosj's profile desrosj Owned by:
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:

Description (last modified by SergeyBiryukov)

This ticket is for various updates and improvements for Core's GitHub Actions workflows.


Change History (11)

#1 @johnbillion
13 months ago

In 55715:

Build/Test Tools: Restrict the permissions granted to jobs on GitHub Actions

The permissions key in a job declares the GitHub permissions that are granted to the token that's used by the job. Restricting the permissions reduces the impact that a vulnerability in the CI system can have.

Props desrosj, johnbillion

See #57865

#2 @SergeyBiryukov
13 months ago

  • Description modified (diff)

This ticket was mentioned in PR #4421 on WordPress/wordpress-develop by @johnbillion.

13 months ago

  • Keywords has-patch added

Trac ticket:

The GitHub Actions workflows are all failing after because the Slack notifications workflows are not being granted the permissions that they require.

Example failing workflow run:

The workflow is not valid. .github/workflows/coding-standards.yml (Line: 172, Col: 3): Error calling workflow 'WordPress/wordpress-develop/.github/workflows/slack-notifications.yml@trunk'. The nested job 'Prepare notifications' is requesting 'actions: read, contents: read', but is only allowed 'actions: none, contents: none'.

#4 @johnbillion
13 months ago

In 55717:

Build/Test Tools: Fix the permissions that are granted to the Slack notifications workflow.

Follow-up to [55715].

See #57865

#6 @johnbillion
13 months ago

  • Type changed from defect (bug) to task (blessed)

This ticket was mentioned in PR #4872 on WordPress/wordpress-develop by @thelovekesh.

10 months ago

Trac ticket:

Add a CI environment variable to notify npm that packages are being installed in a CI environment.

Also see:

@desrosj commented on PR #4872:

10 months ago

Thanks for this, @thelovekesh!

I'll admit, I'd never heard of this before and had to do some reading to make sure I understood it. I'm not convinced that this is a change we should make, but not ready to say no just yet.

I was chatting with @johnbillion about this suggestion, and there's a few things we'd like to look into and have answered:

  • Is it still true that npm does no sniffing to detect common CI environments? It seems a little strange that they're fully relying on this little known/hardly used flag to be defined in order to categorize traffic.
  • Does npm even actively use this? It's still in the documentation, but are they doing anything useful with this information?
  • It's possible that GHA or setup-node is already defining this environment variable. Would be good to confirm whether this is or is not happening. Could render this PR unnecessary.

#9 @desrosj
10 months ago

  • Resolution set to fixed
  • Status changed from new to closed

I'm not yet convinced the attached PR is necessary. There are also some outstanding 3rd-party action updates.

Even though build and test tool changes can be made at any point, I don't feel these are necessary right now. These changes can be included in a grouped backport in the future when deemed necessary.

I've opened #58867 for 6.4.

@thelovekesh commented on PR #4872:

10 months ago

Hey @desrosj,
This PR is already rendered obsolete by the changes to the action runner which adds CI=true in GHA environment. I'll now close this and appreciate you looking into it.

Aside, the latest NPM versions now rely on `ci-info` packages to determine CI environments.

@desrosj commented on PR #4872:

10 months ago

Thanks for circling back! Glad to see that implemented at the platform level. I think it makes much more sense there.

Note: See TracTickets for help on using tickets.