#57865 closed task (blessed) (fixed)
GitHub Actions updates and improvements for 6.3
| Reported by: | | Owned by: | |
|---|---|---|---|
| Milestone: | 6.3 | Priority: | normal |
| Severity: | normal | Version: | |
| Component: | Build/Test Tools | Keywords: | has-patch |
| Focuses: | Cc: |
Description (last modified by )
This ticket is for various updates and improvements for Core's GitHub Actions workflows.
Previously:
- 6.2 (#57572)
Change History (11)
This ticket was mentioned in PR #4421 on WordPress/wordpress-develop by @johnbillion.
3 years ago #3
- Keywords has-patch added
Trac ticket: https://core.trac.wordpress.org/ticket/57865
The GitHub Actions workflows are all failing after https://core.trac.wordpress.org/changeset/55715 because the Slack notifications workflows are not being granted the permissions that they require.
Example failing workflow run: https://github.com/WordPress/wordpress-develop/actions/runs/4877036459
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'.
@johnbillion commented on PR #4421:
3 years ago #5
Thanks David.
Committed in https://core.trac.wordpress.org/changeset/55717
This ticket was mentioned in PR #4872 on WordPress/wordpress-develop by @thelovekesh.
2 years ago #7
Trac ticket: https://core.trac.wordpress.org/ticket/57865
Add a CI environment variable to notify npm that packages are being installed in a CI environment.
2 years ago #8
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-nodeis already defining this environment variable. Would be good to confirm whether this is or is not happening. Could render this PR unnecessary.
#9
@
2 years 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:
2 years ago #10
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.
In 55715: