ci(deploy): move to gha for oidc publishing #4929
Conversation
…to get extra log data in CI.
Garbee
changed the title
|
So, the |
Garbee
dismissed
dbjorge’s stale review
Updated to on push, good catch with the workflow_run only operating from the default branch context.
Scripts almost all extracted into standalone files for the deploy process as well. Only one lingering inline which is just pulling package.json details for validation after production deploy.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 10 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // That means if we try to resolve the import without | ||
| // linking, it will resolve the version in `node_modules` | ||
| // from npm. | ||
| execSync('npm link', execOptions); |
There was a problem hiding this comment.
This is acceptable for the purposes of not blocking this PR, but I think these tests would be more valuable if they used a strategy of "npm pack + set up a test consumer package that refers to the packed tarball", instead of "self-referencing the directory that we built the package from (either via npm link or a self-referencing-import)".
The big difference between those strategies is that the npm pack approach will result in your test only "seeing" the files within the package that we actually publish, and not any of the extra source files/etc which we don't pack into the published package. That creates a more realistic testing environment:
- If we miss an entry in
filesthat's necessary for importing the package to work, thepackversion would catch it but thenpm linkversion would miss it - if we accidentally depend on a
devDependencyat runtime, thepackversion would catch it but thenpm linkversion would pass (because we installed dev dependencies in the source directory in order to build the package)
There was a problem hiding this comment.
(these are real classes of issues we've been bitten by before, not hypotheticals; in early versions of the axe-watcher package, we really did have both of those categories of issues that we only discovered after publishing when we set up example repos outside of the main watcher repo)
There was a problem hiding this comment.
Yea, I'd much rather do testing against an npm pack build. It's only this way now because I was trying replicate the original setup, but do more expansive testing as well while I was in it.
I also want to pilot npm pack based testing in other internal repositories first to make sure we have proper patterns down before doing it in this repository.
Garbee
dismissed
dbjorge’s stale review
Old SRI validation path added to tests to maintain that parity.
Fixed the API call so we sort after to ensure the consistency.
While I was in that script, added debug logging to the step summary. Also fixed the time handling so it is told the total runtime it has and the attempts are calculated from that. Reducing the variables exposed and making it easier to consume.
Shellcheck ran on all the scripts and fixed the errors. Along with the pipefail addition.
dbjorge
left a comment
dbjorge
left a comment
There was a problem hiding this comment.
Changes look good to me! Leaving security review unchecked so an ocarina-team member can do a final validation. If you haven't already, it'd also be a good idea to test out the new scripts/workflow steps in a separate test repo before merging.
|
Last patches apply two key fixes. First, anytime we use the GH CLI in workflows, we must pass the token as I found those two when running fully in a separate repository for testing. I also hard coded a validation to check 4.11.0 from NPM. So the post validation script is confirmed working as-is without any further modifications. |
There was a problem hiding this comment.
So this may be an unfounded concern, but wanted to make sure we were OK with its implications.
Right now all the release logic and handling is now written in two places (github action logic and npm scripts) whereas before it was written in a single place (npm scripts), and now some of that logic is completely removed from npm scripts (the next_release script). What I'm afraid of is this will present problems if we ever have to release manually and not using github actions (say github actions are down and we need to get a deploy out). If the scripts are separate that means we'll likely only make fixes to the deploy in the github actions part and not in the npm script part, so come time to manually deploy we will run into tons of errors since we haven't made updates there (this isn't a hypothetical either as we've had to manually create releases for axe-core before).
It also creates two different places we have to remember to make changes (for example remembering that sri validation happens in the sri-valdiate npm script and also in the validate-package.mjs script as it has it's own logic.
@dbjorge do you feel this is a reasonable worry?
This is functionally not possible now. We'd have to wait for Actions to come back up. In what scenario has something come up before that necessitated such a rapid deployment? The whole point of moving to OIDC deployment and keeping things ran through a consistent pipeline. No one should ever be deploying manually. That's a major problem.
This is a very specific issue, which we discussed in one of the comment threads. We're going to look into unifying all of this and testing against and What potential issues do you see with the SRI validation coming up? (Which, is itself deprecated to begin with.) |
|
I agree with the principle of what you're saying, @straker, that we want it to be possible to manually release if we absolutely have to (it'd require temporarily modifying the npmjs.com settings for the package to allow a non-OIDC publish, which is good and intentional), but that said: I'm not seeing why that would become any more or less difficult with these changes, given that @Garbee has accepted the feedback already to move complicated scripting behavior into standalone scripts rather than embedding in the actions yaml. The process before would have been "run the circleci
I think the most relevant scenario for "why would we want to be able to release independently of GitHub actions" would be "suppose a Deque org admin's GitHub credentials become compromised, and a bad actor uses that to publish a malicious version of axe-core and then lock us out of GitHub; we'd want a way to rapidly revoke OIDC access and publish a non-malicious version of axe-core without blocking on getting GitHub access restored" |
Background
In order to continue publishing to npm, the org is moving to OIDC tokens. This is setup through GitHub Actions. Which has prompted a conversion from CircleCI to GHA for this repository. In this the two previous PRs, #4918 and #4919, have slowly ported chunks of the dependency workflows over. This PR now provides the deployment support to ship changes from GHA.
Key Changes
Addition of Deployment Workflow
Concurrency: One queue per branch. Does not cancel previous runs.
The deployment workflow is triggered only on
developandmastercommits once the tests have completed. First, the status of the tests is checked before proceeding with any work. If any failures happened, this workflow does not do anything.First, the "Tests" workflow for the associated commit is waited on. About 12 minutes (overhead for network slowness) is allocated, 14 total runtime minutes on the job before it is force killed. If the workflow was successful, the deployment for the branch in question begins. If any other conclusion is found, or none at the end of the timer, the job exits with an error so no deployment occurs.
If the branch is
develop, execution runs autonomously to build and test the package before publishing. Once all is successful, a publish of the next tag is conducted.If the branch is
master, execution starts with theprod-holdjob. This is a job that accesses an environment that is defined with "Required Reviewers". Meaning it will not allow the job to continue until someone on that reviewer list has approved it to happen.Once permission is granted, the
prod-deployjob kicks off. Conducting the same actions as the next release does, but no special tags on the version which makes itlatestand stable. Once the publish is out, some key information is retrieved as output for the job.Once a successful
prod-deployhas happened, two jobs kick off. One creates the GitHub Release, which uses the same script as was used in CircleCI. The other sets up NodeJS, waits for the package to be visible on npm if it isn't already, then installs the package globally. After global installation finishes, a few tests of the package that was deployed are run to ensure it is functional.A key part of both workflows, is ensuring the package contents look correct before publishing. To support more expansive testing of the required behavior, a new node script is introduced. Which helps separate the pre-publish and post-publish testing. As well as expands the capability of that test to be more comprehensive.
New Environment
A new environment is introduced,
production-deploy. This is restricted to only being accessible from themasterbranch. It is configured with required reviewers. So before the environment can be accessed in a workflow run, someone from the required reviewer group must manually approve the access in the GitHub Actions UX.This secondary environment is needed since we auto-deploy
nexttags off thedevelopbranch. But formasterwith stable deploys, we want to have manual approval before it goes out. Since we can't add the required reviewers to the main environment as that would impactdevelop, this is made to target just stable releases.New pre-publish validation script
CircleCI is configured with a shell script that did some pre and post deploy validation of the contents. While it is effective for the basics, it was fairly minimal in what it did as a whole. This now has a far more comprehensive suite of checks that runs for this step.
The new script pulls key data from the package to build a report. It checks that all contents of the
filesdefinition exist on the filesystem. It then checks to ensure that the package can be imported in CommonJS. After that, ensuring it is importable (along with all importable files shipped) are capable of that in ESM. Finally, it validates the SRI hashes defined in the sri history are what are computed from the current version. The SRI check only occurs onmasterandrelease-branches. As ondevelopthe hash is not updated with every change.This script works by running as a Node ESM script. It pulls key information, does the filesystem check, then links the package to itself. This makes resolving the package pull the linked version on the filesystem instead of the version installed by
@axe-core/webdriverjsfor integration testing.The output of this is logged to the console and compiled into a more easily viewable step summary. The successful output of which can be seen in a comment on this PR.
Addition of final tests before deployment of stable
One test not added previously,
test_rule_help_version, which checks for the docs to be live; is now added. As this only impacted stable releases going out, it made sense to reduce the initial scope and introduce this here.The next test added was an explicit SRI validation. This runs on all commits to
masterandrelease-*branches as well as all PRs targeting them. It uses the old validation pathway for quickly getting the check back in place. We can look into what to do with the SRI stuff itself later.Removal of CircleCI Publishing Configuration
Within this patch, the CircleCI configuration to deploy to NPM is removed since we now want to run only from OIDC on GitHub Actions. The full test suite is not removed with this. It will be removed shortly after this lands when we convert the required checks over to GHA for merging.
Visual Overviews
Deploy Workflow
Workflow Control Color Coding#e1f5ff#ffe1e1#e1ffe1#fff4e1#b3d9ff#d9b3ff#ffb3d9#b3ffb3#ffd9b3Click to open diagram
flowchart TD Start([Push to master/develop]) --> WaitTests[wait-for-tests Job] WaitTests --> WT1[Checkout Code] WT1 --> WT2[Wait for Tests Workflow] WT2 --> TestSuccess{Tests Successful?} TestSuccess -->|No| End([End - Tests Failed]) TestSuccess -->|Yes| BranchCheck{Which Branch?} BranchCheck -->|develop| DeployNext[deploy-next Job] BranchCheck -->|master| ProdHold[prod-hold Job] DeployNext --> DN1[Checkout Code] DN1 --> DN2[Install Dependencies] DN2 --> DN3[Build Package<br/>npm prepare & build] DN3 --> DN4[Determine Prerelease Version] DN4 --> DN5[Bump Version] DN5 --> DN6[Validate Package] DN6 --> DN7[Publish to NPM<br/>--tag=next] DN7 --> ValidateNextDeploy[validate-next-deploy Job] ValidateNextDeploy --> VND1[Checkout Code] VND1 --> VND2[Setup Node.js] VND2 --> VND3[Wait for Package on NPM] VND3 --> VND4[Validate Installation of next] VND4 --> EndNext([End - Next Version Validated]) ProdHold --> PH1[Manual Approval Gate] PH1 --> ProdDeploy[prod-deploy Job] ProdDeploy --> PD1[Checkout Code] PD1 --> PD2[Install Dependencies] PD2 --> PD3[Build Package<br/>npm prepare & build] PD3 --> PD4[Validate Package] PD4 --> PD5[Publish to NPM<br/>stable version] PD5 --> PD6[Get Package Data<br/>version & name] PD6 --> CreateRelease[create-github-release Job] PD6 --> ValidateDeploy[validate-deploy Job] CreateRelease --> CR1[Checkout Code] CR1 --> CR2[Install Release Helper<br/>github-release tool] CR2 --> CR3[Download Release Script] CR3 --> CR4[Make Script Executable] CR4 --> CR5[Create GitHub Release] CR5 --> EndRelease([GitHub Release Created]) ValidateDeploy --> VD1[Checkout Code] VD1 --> VD2[Setup Node.js] VD2 --> VD3[Wait for Package on NPM] VD3 --> VD4[Validate Installation of Stable] VD4 --> EndValidate([End - Deploy Validated]) %% Consistent step colors with accessible text style DN1 fill:#b3d9ff,color:#001a33,stroke:#0066cc style PD1 fill:#b3d9ff,color:#001a33,stroke:#0066cc style CR1 fill:#b3d9ff,color:#001a33,stroke:#0066cc style WT1 fill:#b3d9ff,color:#001a33,stroke:#0066cc style VND1 fill:#b3d9ff,color:#001a33,stroke:#0066cc style VD1 fill:#b3d9ff,color:#001a33,stroke:#0066cc style DN2 fill:#d9b3ff,color:#1a0033,stroke:#6600cc style PD2 fill:#d9b3ff,color:#1a0033,stroke:#6600cc style DN3 fill:#ffb3d9,color:#330011,stroke:#cc0066 style PD3 fill:#ffb3d9,color:#330011,stroke:#cc0066 style DN6 fill:#b3ffb3,color:#002200,stroke:#00aa00 style PD4 fill:#b3ffb3,color:#002200,stroke:#00aa00 style DN7 fill:#ffd9b3,color:#331100,stroke:#cc6600 style PD5 fill:#ffd9b3,color:#331100,stroke:#cc6600 %% Decision/Gate styling style Start fill:#e1f5ff,color:#001a33 style End fill:#ffe1e1,color:#330000 style EndNext fill:#e1ffe1,color:#002200 style EndRelease fill:#e1ffe1,color:#002200 style EndValidate fill:#e1ffe1,color:#002200 style PH1 fill:#fff4e1,color:#331100 style TestSuccess fill:#fff4e1,color:#331100 style BranchCheck fill:#fff4e1,color:#331100Validation Script
Color Coding of Scopes#1a1a1a#004d99#664d00#4d004d#004d4d#5c5c5c#2d5016#7d1007#5c5c5cClick to open diagram
flowchart TD Start([Start Script]) --> Init[Initialize Variables] Init --> FileCheck[File Existence Check] FileCheck --> FileLoop[For each file in pkg.files array] FileLoop --> FileExists{File exists?} FileExists -->|yes| FilePass[✓ Mark as Found] FileExists -->|no| FileFail[✗ Mark as Missing<br/>exitCode++] FilePass --> LinkSetup FileFail --> LinkSetup LinkSetup[Create npm link] -->|success| CJS[CommonJS Compatibility Check] LinkSetup -->|fail| LinkError[Log error and skip remaining checks] CJS --> CJSRequire{Require package successful?} CJSRequire -->|yes| CJSValidate{Validate export type and version exists?} CJSRequire -->|no| CJSFail[✗ CommonJS Failed<br/>exitCode++] CJSValidate -->|yes| CJSPass[✓ CommonJS Compatible] CJSValidate -->|no| CJSFail CJSPass --> Import CJSFail --> Import Import[Importable Check] --> ImportMain{Import main package?} ImportMain -->|success| ValidateMainVersion{Version property exists?} ImportMain -->|fail| ImportMainFail[✗ Not Importable<br/>anyCaught = true] ValidateMainVersion -->|yes| ImportMainPass[✓ Mark as Importable] ValidateMainVersion -->|no| ImportMainFail ImportMainPass --> ImportLoop ImportMainFail --> ImportLoop ImportLoop[For each file in pkg.files] --> FileType{File type?} FileType -->|skip .txt, .d.ts, folders| CheckAnyCaught FileType -->|check resolve| NodeModCheck{Resolves to node_modules?} NodeModCheck -->|yes| ImportNodeFail[✗ Resolves to node_modules<br/>exitCode++] NodeModCheck -->|no| ImportTry{Import successful?} ImportTry -->|yes| CheckVersion{Has version property?} ImportTry -->|no| ImportFileFail[✗ Not Importable<br/>anyCaught = true] CheckVersion -->|yes| ImportPass[✓ Importable] CheckVersion -->|no| ImportFileFail ImportPass --> CheckAnyCaught ImportFileFail --> CheckAnyCaught ImportNodeFail --> CheckAnyCaught CheckAnyCaught{anyCaught true?} -->|yes| ImportExit[exitCode++] CheckAnyCaught -->|no| SRICheck ImportExit --> SRICheck SRICheck[SRI Hash Validation] --> BranchCheck{Branch is master or release-*?} BranchCheck -->|no| SkipSRI[Skip SRI validation] BranchCheck -->|yes| SRILoad[Load sri-history.json<br/>Calculate hashes for axe.js and axe.min.js] SRILoad --> SRICompare{Hash matches expected?} SRICompare -->|yes| SRIPass[✓ Valid SRI] SRICompare -->|no| SRIFail[✗ Invalid SRI<br/>exitCode++] SRIPass --> Cleanup SRIFail --> Cleanup SkipSRI --> Cleanup Cleanup[Unlink npm package] --> End([Exit with exitCode]) LinkError --> CleanupError[Attempt unlink if needed] CleanupError --> End style Start fill:#1a1a1a,stroke:#000,stroke-width:2px,color:#fff style Init fill:#1a1a1a,stroke:#000,stroke-width:2px,color:#fff style End fill:#1a1a1a,stroke:#000,stroke-width:2px,color:#fff style FileCheck fill:#004d99,stroke:#000,stroke-width:2px,color:#fff style FileLoop fill:#004d99,stroke:#000,stroke-width:2px,color:#fff style FileExists fill:#004d99,stroke:#000,stroke-width:2px,color:#fff style FilePass fill:#2d5016,stroke:#000,stroke-width:2px,color:#fff style FileFail fill:#7d1007,stroke:#000,stroke-width:2px,color:#fff style LinkSetup fill:#1a1a1a,stroke:#000,stroke-width:2px,color:#fff style LinkError fill:#7d1007,stroke:#000,stroke-width:2px,color:#fff style CJS fill:#664d00,stroke:#000,stroke-width:2px,color:#fff style CJSRequire fill:#664d00,stroke:#000,stroke-width:2px,color:#fff style CJSValidate fill:#664d00,stroke:#000,stroke-width:2px,color:#fff style CJSPass fill:#2d5016,stroke:#000,stroke-width:2px,color:#fff style CJSFail fill:#7d1007,stroke:#000,stroke-width:2px,color:#fff style Import fill:#4d004d,stroke:#000,stroke-width:2px,color:#fff style ImportMain fill:#4d004d,stroke:#000,stroke-width:2px,color:#fff style ValidateMainVersion fill:#4d004d,stroke:#000,stroke-width:2px,color:#fff style ImportMainPass fill:#2d5016,stroke:#000,stroke-width:2px,color:#fff style ImportLoop fill:#4d004d,stroke:#000,stroke-width:2px,color:#fff style FileType fill:#4d004d,stroke:#000,stroke-width:2px,color:#fff style NodeModCheck fill:#4d004d,stroke:#000,stroke-width:2px,color:#fff style ImportTry fill:#4d004d,stroke:#000,stroke-width:2px,color:#fff style CheckVersion fill:#4d004d,stroke:#000,stroke-width:2px,color:#fff style CheckAnyCaught fill:#4d004d,stroke:#000,stroke-width:2px,color:#fff style ImportExit fill:#4d004d,stroke:#000,stroke-width:2px,color:#fff style ImportPass fill:#2d5016,stroke:#000,stroke-width:2px,color:#fff style ImportMainFail fill:#7d1007,stroke:#000,stroke-width:2px,color:#fff style ImportFileFail fill:#7d1007,stroke:#000,stroke-width:2px,color:#fff style ImportNodeFail fill:#7d1007,stroke:#000,stroke-width:2px,color:#fff style SRICheck fill:#004d4d,stroke:#000,stroke-width:2px,color:#fff style BranchCheck fill:#004d4d,stroke:#000,stroke-width:2px,color:#fff style SRILoad fill:#004d4d,stroke:#000,stroke-width:2px,color:#fff style SRICompare fill:#004d4d,stroke:#000,stroke-width:2px,color:#fff style SRIPass fill:#2d5016,stroke:#000,stroke-width:2px,color:#fff style SRIFail fill:#7d1007,stroke:#000,stroke-width:2px,color:#fff style SkipSRI fill:#5c5c5c,stroke:#000,stroke-width:2px,color:#fff style Cleanup fill:#1a1a1a,stroke:#000,stroke-width:2px,color:#fff style CleanupError fill:#1a1a1a,stroke:#000,stroke-width:2px,color:#fffFixes: #4912