NEW @W-17330636@ Implemented SNAPSHOT-based release cycle #152

jfeingold35 · 2024-12-06T20:12:44Z

No description provided.

jfeingold35 · 2024-12-06T20:15:16Z

.github/workflows/verify-pr/validate-package-interdependencies.js

Has no hardcoded package names, so no changes are necessary to add new packages to the repo as long as they're located in the packages folder with the others.

jfeingold35 · 2024-12-06T20:17:54Z

.github/workflows/verify-pr/validate-changed-package-versions.js

Has no hardcoded package names, so requires no changes to work with other packages if/when we add them.

jfeingold35 · 2024-12-06T20:20:43Z

.github/workflows/verify-pr/validate-package-interdependencies.js

+    return JSON.parse(fs.readFileSync(packageJsonPath, 'utf-8'));
+}
+
+function identifyIncorrectlyInterdependentPackages(packageJsons) {


Note: This method merely enforces that packages depend on the latest version of each other (e.g., if package-a is at 0.10.0, then package-b can't depend on 0.9.0).
It doesn't currently enforce that all the engines have the same minor version number, because that seemed like it would be excessive and possibly inconvenient. I'm open to adding that enforcement if desired.

Yeah we won't want all engines have the same minor version. As soon as we go GA... and the version numbers at 1.0.0 then we'll be updating minor versions and patch numbers based on standard SEMVER practices. Thus the version numbers of each of the engines will start to diverge from one another.

We only keep them lockstep right now because it helped to know that we updated all of them. But now that we have these scripts - we can more safely go back to best practices.

@stephen-carter-at-sf , I suspect that once we go GA, we might want to add enforcement to make sure that the API and all the implementations are on the same Major Version, if nothing else. We'll see when we get there, though.

jfeingold35 · 2024-12-06T20:22:24Z

.github/workflows/verify-pr.yml

@@ -15,12 +15,52 @@ jobs:
        if: github.base_ref == 'dev'
        run: |
          title="${{ github.event.pull_request.title }}"
-          if [[ "$title" =~ ^(FIX|CHANGE|NEW)([[:space:]]*\([^()]+\))?[[:space:]]*:?[[:space:]]*@W-[[:digit:]]{8,9}@[[:space:]]*.+ ]]; then
+          if [[ "$title" =~ ^(POSTRELEASE|FIX|CHANGE|NEW)([[:space:]]*\([^()]+\))?[[:space:]]*:?[[:space:]]*@W-[[:digit:]]{8,9}@[[:space:]]*.+ ]]; then


New "POSTRELEASE" keyword allows the bypassing of version and dependency validation.

jfeingold35 · 2024-12-06T20:23:26Z

.github/workflows/verify-pr.yml

+          fetch-depth: 0
+      - uses: actions/setup-node@v4
+        with:
+          node-version: 'lts/*'


Note: I had us use lts/* anywhere that I called actions/setup-node@v4. Is there a reason we were using 20 that justifies continuing to do so, even though LTS is now 22?

For build validation stuff - I don't think it matters. But for testing, we want to make sure all tests pass on v20+ to ensure we don't accidentally use a new feature that breaks users that are still on node 20.

@stephen-carter-at-sf , question: In 5 months when v18 gets End-Of-Lifed, will we want to change our builds from using 20 to 22? Or are we on 20 until 20 gets End-Of-Lifed in 2026?

jfeingold35 · 2024-12-06T21:05:25Z

.github/workflows/publish-package-to-npm.yml

+        id: create-release-branch
+        run: |
+          NOW_TIMESTAMP=$(date +%s)
+          git checkout -b release/$NOW_TIMESTAMP


sfdx-scanner uses release/vX.Y.Z, but we can't do that here since there's no single version to describe the state of the ecosystem. As such, a Unix timestamp seems like a reasonable way to make sure that each branch has a unique name, and that the branches can be sorted from oldest to newest if needed.

jfeingold35 · 2024-12-06T21:09:43Z

.github/workflows/publish-package-to-npm.yml

+          echo "branch_name=release/$NOW_TIMESTAMP" >> "$GITHUB_OUTPUT"
+      - name: Strip '-SNAPSHOT' from to-be-released package versions
+        run: |
+          if [ "${{ inputs.code-analyzer-core }}" == "true" ]; then


Every package has its own hardcoded if-else here. This was the naive implementation. If y'all think it's worth pursuing, I could probably do something clever with iterating over a list to remove the hardcoding.

jfeingold35 · 2024-12-06T21:13:13Z

.github/workflows/publish-package-to-npm.yml

+        run: |
+          if [ "${{ inputs.code-analyzer-core }}" == "true" ]; then
+            cd ./packages/code-analyzer-core
+            npm --no-git-tag-version version patch # Increments X.Y.Z-SNAPSHOT to X.Y.Z, which is what we want.


When a SemVer has a suffix (e.g., -SNAPSHOT), then npm version patch just pops the suffix off and leaves all of the numbers unchanged. That, to me, seemed better than having a picklist-input for "minor/patch", because it allows each package to increment its version independently, which could be desirable.
The downside is that we have to choose what new version we're snapshotting when we add the suffix back during development (e.g., if we release 0.14.0, then we need to decide whether the next version should be 0.15.0-SNAPSHOT or 0.14.1-SNAPSHOT).

jfeingold35 · 2024-12-06T21:15:31Z

.github/workflows/publish-package-to-npm.yml

+          # GraphQL needs a message for the commit.
+          MESSAGE="Preparing Core Ecosystem for release"
+          # GraphQL needs the latest versions of all the package.json files, as Base64 encoded strings.
+          CORE_PACKAGE_JSON="$(cat packages/code-analyzer-core/package.json | base64)"


I had no desire to try dynamically generating the GraphQL query, so I just added one variable per package JSON (meaning that new packages will require new variables), and add them all to the commit. Adding them all regardless of whether they changed should be fine, because an unchanged file would just be a no-op (according to my understanding).

jfeingold35 · 2024-12-06T21:24:11Z

.github/workflows/publish-package-to-npm.yml

-          npm run test
-      - name: publish-to-npm
+          npm run test # Test here, since we didn't do it elsewhere
+      - name: Publish Engine API


Each package is published with its own hardcoded step within one job, as opposed to using a matrix to create multiple jobs.
Upsides:

Easy to make sure that code-analyzer-engine-api gets published first if necessary

Only need to call npm run build/npm run test once

Downsides:

Each package requires its own hardcoded step

Since all publishes share the same job, the process of retrying a partially successful publish (e.g., 6 out of 7 succeed) is a little bit more complicated. Still entirely doable, though

jfeingold35 · 2024-12-06T21:28:32Z

.github/workflows/verify-pr.yml

+        with:
+          node-version: 'lts/*'
+      - name: Validate that changed packages are versioned as snapshots
+        if: ${{ needs.check_for_postrelease_keyword.outputs.is-postrelease == 'false' }}


This step and the next one have the is-postrelease == false check, because putting that check at the job-level would prevent us from requiring this job in the branch protection rule.

jag-j · 2024-12-06T22:31:55Z

.github/workflows/publish-package-to-npm.yml

+          if [ "${{ inputs.code-analyzer-core }}" == "true" ]; then
+            PACKAGES_TO_CHECK+=('code-analyzer-core')
+          fi
+          if [ "${{ inputs.code-analyzer-engine-api }}" == "true" ]; then


The check for each of the packages is very verbose. Can you have the package names in an array and just loop through them instead? For each of the array element, you could do something like the below:
PACKAGES_TO_CHECK+=("code-analyzer-${PACKAGE}").
This would greatly reduce the code size.

Should be doable. I think I can also do something similar at line 113. I'll give it a shot and see how it goes.

@jag-j , I've done some digging, and it looks like I have to reference the inputs to the Github Action as hardcoded strings (e.g., I have to do ${{ inputs.code-analyzer-core }}, and can't do something like ${{ inputs['code-analyzer-' + 'core'] }}.
So I can't get rid of the if-chain in the first step, but I might be able to get rid of the other ones by looping over the PACKAGES_TO_CHECK array in subsequent steps. I'll try that and let you know how it goes.

Okay, I wasn't able to get rid of it here, but I was able to get rid of it at all of the later steps except for the GraphQL query construction (I really don't want to mess with that unless I have to)

jfeingold35 · 2024-12-09T19:23:21Z

.github/workflows/publish-package-to-npm.yml

-          cd ../..
-      - name: Publish ESLint Engine
-        if: ${{ inputs.code-analyzer-eslint-engine == 'true' }}
+      - name: Publish Packages


All packages are published in a single step. The alternative would have been to experiment with dynamic job matrixing and give each publish its own individual job.

Upsides of the current implementation:

The code is very tight and easy to read

Requires no changes to support the addition of future packages

Extremely easy to sequence package publishing as desired, no matter which packages are being published. (We just make sure the Engine API package is at the start of the list, and we're good)

Downsides of the current implementation:

Packages are published sequentially. If we instead used a Job Matrix, then we could have the Engine API publish first and then have all the other packages publish in parallel.

Can't easily retry publishing an individual package that failed to publish (e.g., if we publish 5 packages, and the 5th fails because NPM timed out, then we can't just click to retry the way we can with a failed job. We'll need a pull request to de-snapshot the successfuly published packages, and then we'll need to re-run the entire workflow on the new commit.)

We need engine-api to go first since npm will complain if another package references it and its version doesn't exist.

So are we controlling the order somehow to make the engine-api package always go out first if there is a change to it?

@stephen-carter-at-sf , Yeah, the validate-packages-as-releasable job constructs the string that the rest of the jobs use to know which packages they care about, and it starts with code-analyzer-engine-api. So that one will always be first on the list if it's being published at all, therefore guaranteeing it gets published first.

stephen-carter-at-sf · 2024-12-09T19:29:39Z

.github/workflows/verify-pr.yml

+          fetch-depth: 0
+      - uses: actions/setup-node@v4
+        with:
+          node-version: 'lts/*'


For build validation stuff - I don't think it matters. But for testing, we want to make sure all tests pass on v20+ to ensure we don't accidentally use a new feature that breaks users that are still on node 20.

stephen-carter-at-sf · 2024-12-09T19:40:51Z

.github/workflows/verify-pr/validate-package-interdependencies.js

+    return JSON.parse(fs.readFileSync(packageJsonPath, 'utf-8'));
+}
+
+function identifyIncorrectlyInterdependentPackages(packageJsons) {


Yeah we won't want all engines have the same minor version. As soon as we go GA... and the version numbers at 1.0.0 then we'll be updating minor versions and patch numbers based on standard SEMVER practices. Thus the version numbers of each of the engines will start to diverge from one another.

We only keep them lockstep right now because it helped to know that we updated all of them. But now that we have these scripts - we can more safely go back to best practices.

stephen-carter-at-sf · 2024-12-09T19:49:23Z

.github/workflows/publish-package-to-npm.yml

-          cd ../..
-      - name: Publish ESLint Engine
-        if: ${{ inputs.code-analyzer-eslint-engine == 'true' }}
+      - name: Publish Packages


We need engine-api to go first since npm will complain if another package references it and its version doesn't exist.

So are we controlling the order somehow to make the engine-api package always go out first if there is a change to it?

stephen-carter-at-sf · 2024-12-09T19:53:34Z

.github/workflows/publish-package-to-npm.yml

    runs-on: ubuntu-latest
-    defaults:
-      run:
-        working-directory: ./packages/${{inputs.package}}


OK - super important... we can't lose this. We need to make sure that we are publishing one package at a time from within the package and not at the mono-repo level. This ensures that the package has its dependencies set correctly.

That is, when dealing with monorepos... all packages all share the same node_modules folder. So theoretically 1 package could depend on module A... and another package also depends on module A but forgets to depend on it and the tests pass if we build/test at the monorepo level because the first package brought it in.

So we need to build/test/package in isolation... which means we need to act as if we are not in a monorepo - by making the workspace (pwd) equal to the package directory individually.

@stephen-carter-at-sf , Alright, so I could iterate over all the packages in the repo, cd into the folder, and then run npm install/run build/run test from there, and that would work, right?

yes that should work.

@stephen-carter-at-sf , done; see line 227.

jfeingold35 force-pushed the d/W-17330636 branch 2 times, most recently from 70e120b to 1e1a8f7 Compare December 6, 2024 20:24

jfeingold35 commented Dec 6, 2024

View reviewed changes

jag-j reviewed Dec 6, 2024

View reviewed changes

jfeingold35 commented Dec 9, 2024

View reviewed changes

stephen-carter-at-sf requested changes Dec 9, 2024

View reviewed changes

jfeingold35 force-pushed the d/W-17330636 branch from 3656d64 to 75a8beb Compare December 9, 2024 20:25

stephen-carter-at-sf approved these changes Dec 9, 2024

View reviewed changes

jfeingold35 added 5 commits December 9, 2024 14:34

NEW @W-17330636@ Implemented SNAPSHOT-based release cycle

5752055

NEW @W-17330636@ Minor correction to workflow

a66ef74

@W-17330636@ Minimized hardcoded package name use

c036e9d

@W-17330636@ Integrated feedback from code review

765324e

@W-17330636@ Integrated feedback from code review

0f35752

jfeingold35 force-pushed the d/W-17330636 branch from 1c998f6 to 0f35752 Compare December 9, 2024 20:35

jfeingold35 merged commit 02fb0d1 into dev Dec 9, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW @W-17330636@ Implemented SNAPSHOT-based release cycle #152

NEW @W-17330636@ Implemented SNAPSHOT-based release cycle #152

jfeingold35 commented Dec 6, 2024

jfeingold35 Dec 6, 2024

jfeingold35 Dec 6, 2024

jfeingold35 Dec 6, 2024

stephen-carter-at-sf Dec 9, 2024

jfeingold35 Dec 9, 2024

jfeingold35 Dec 6, 2024

jfeingold35 Dec 6, 2024

stephen-carter-at-sf Dec 9, 2024

jfeingold35 Dec 9, 2024

jfeingold35 Dec 6, 2024

jfeingold35 Dec 6, 2024

jfeingold35 Dec 6, 2024

jfeingold35 Dec 6, 2024

jfeingold35 Dec 6, 2024

jfeingold35 Dec 6, 2024

jag-j Dec 6, 2024

jfeingold35 Dec 6, 2024

jfeingold35 Dec 9, 2024

jfeingold35 Dec 9, 2024

jfeingold35 Dec 9, 2024

stephen-carter-at-sf Dec 9, 2024

jfeingold35 Dec 9, 2024

stephen-carter-at-sf Dec 9, 2024

stephen-carter-at-sf Dec 9, 2024

stephen-carter-at-sf Dec 9, 2024

stephen-carter-at-sf Dec 9, 2024

jfeingold35 Dec 9, 2024

stephen-carter-at-sf Dec 9, 2024

jfeingold35 Dec 9, 2024

NEW @W-17330636@ Implemented SNAPSHOT-based release cycle #152

NEW @W-17330636@ Implemented SNAPSHOT-based release cycle #152

Conversation

jfeingold35 commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment