-
Notifications
You must be signed in to change notification settings - Fork 32
Detect Changes based on Lockfile #484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
leeyi45
wants to merge
48
commits into
master
Choose a base branch
from
workspaces-fix
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
48 commits
Select commit
Hold shift + click to select a range
a0ff005
Fix topo sort trying to read from packages that aren't within the rep…
leeyi45 9289750
Fix incorrect documentation url references
leeyi45 b4e6444
Fix coverage also including test utility files
leeyi45 362de6a
Fix devserver vite config not being linted
leeyi45 d94b6e4
Fix incorrect building commands for devserver
leeyi45 d717438
Fix the artifact command
leeyi45 abce599
Update workflows to not execute jobs at all if there were no detected…
leeyi45 d95597d
Update matrix jobs to properly not execute when they don't have to
leeyi45 7015382
Make devserver and docserver still execute even if the tabs or librar…
leeyi45 cc8ac68
Make end github job execute properly
leeyi45 a5c3d16
Remove trailing spaces
leeyi45 2eb8bed
Test including lockfile information for hasChanges
leeyi45 d08cb16
Linting and lodash import path fixes
leeyi45 f767caf
Fix incorrect package name processing
leeyi45 18fc8ef
Fix broken package names being used in commands
leeyi45 792d1ac
Add tests for lockfile functions
leeyi45 80d871a
Minor documentation fixes
leeyi45 69e79bd
Add tests to command options
leeyi45 015a6c7
FIx missing icons
leeyi45 e8fde17
Merge
leeyi45 72f3f0a
Use Richard's implementation of lockfile change detection
leeyi45 9f5e8b8
Merge commit '8405f6cbc4edc432b7215f04acae9f40d117436a' into workspac…
leeyi45 921ad38
Fix typo
leeyi45 b5bc478
Fix incorrect null check
leeyi45 42f7280
Update some documentation
leeyi45 2376085
Update tests
leeyi45 ac74434
Merge branch 'workspaces-fix' of github.com:source-academy/modules in…
leeyi45 be83db9
Merge commit 'fea3108fedbc8c779638ac0202083b07c1b6bc7d' into workspac…
leeyi45 9e579cb
Merge branch 'workspaces-fix' of github.com:source-academy/modules in…
leeyi45 34caab0
Update some incorrect documentation
leeyi45 3d4bcde
Merge branch 'master' into workspaces-fix
RichDom2185 0d9c1e9
Fix not being able to add extra options when declaring dirtree code f…
leeyi45 6f10600
Add highlighting to directory trees
leeyi45 276b193
Run linting
leeyi45 2b36efb
Merge branch 'workspaces-fix' of github.com:source-academy/modules in…
leeyi45 c7a5baf
Memoize calls to yarn why for lockfile checking
leeyi45 cc3ebe0
Merge branch 'master' into workspaces-fix
RichDom2185 64c2b03
Merge branch 'master' into workspaces-fix
RichDom2185 a8a5c7b
Merge branch 'master' into workspaces-fix
RichDom2185 52ee6da
Move tsc utilities to repotools
leeyi45 0da7a5b
Rename the tsc functions
leeyi45 35a2154
Relocate builders to repotools
leeyi45 927c02f
Finish migrating repotools
leeyi45 87f3849
Fix some duplicated code in repotools
leeyi45 8002f00
Remove obsolete option from markdown-tree and get tsconfig to include…
leeyi45 87cc784
do a merge
leeyi45 d844e16
Relocate and re-type building code
leeyi45 8175605
Continue fixing buildtools
leeyi45 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import { describe, expect, it, vi } from 'vitest'; | ||
| import * as utils from '../lockfiles.js'; | ||
|
|
||
| vi.mock(import('../gitRoot.js'), () => ({ | ||
| gitRoot: 'root' | ||
| })); | ||
|
|
||
| describe(utils.extractPackageName, () => { | ||
| it('works with packages that start with @', () => { | ||
| expect(utils.extractPackageName('@sourceacademy/tab-Rune@workspace:^')) | ||
| .toEqual('@sourceacademy/tab-Rune'); | ||
| }); | ||
|
|
||
| it('works with regular package names', () => { | ||
| expect(utils.extractPackageName('lodash@npm:^4.17.20')) | ||
| .toEqual('lodash'); | ||
| }); | ||
|
|
||
| it('throws on invalid package name', () => { | ||
| expect(() => utils.extractPackageName('something weird')) | ||
| .toThrowError('Invalid package name: something weird'); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| {"value":"@sourceacademy/bundle-unittest@workspace:src/bundles/unittest","children":{"lodash@npm:4.17.21":{"locator":"lodash@npm:4.17.21","descriptor":"lodash@npm:^4.17.21"}}} | ||
| {"value":"@sourceacademy/markdown-plugin-directory-tree@virtual:f0562030a653a107496800a80ececfadfd737a6a260bbd2572ca4e42de1d6b97749197bb1c41d0d71c9f18ab009789560666041994678fe1aba63902e9fb15bd#workspace:lib/markdown-tree","children":{"lodash@npm:4.17.21":{"locator":"lodash@npm:4.17.21","descriptor":"lodash@npm:^4.17.21"}}} | ||
| {"value":"@sourceacademy/markdown-plugin-directory-tree@workspace:lib/markdown-tree","children":{"lodash@npm:4.17.21":{"locator":"lodash@npm:4.17.21","descriptor":"lodash@npm:^4.17.21"}}} | ||
| {"value":"@sourceacademy/modules-buildtools@workspace:lib/buildtools","children":{"lodash@npm:4.17.21":{"locator":"lodash@npm:4.17.21","descriptor":"lodash@npm:^4.17.21"}}} | ||
| {"value":"@sourceacademy/modules-github-actions@workspace:.github/actions","children":{"lodash@npm:4.17.21":{"locator":"lodash@npm:4.17.21","descriptor":"lodash@npm:^4.17.21"}}} | ||
| {"value":"@sourceacademy/modules-repotools@virtual:1448f4d06c0cf02cc986bce0139b31c0f3c95af1573e69673bc11ad2928a2fc505818a40d3f1caf168c35ce00c20238d52dfdba1940c59c81fd5df1b08ba49d6#workspace:lib/repotools","children":{"lodash@npm:4.17.21":{"locator":"lodash@npm:4.17.21","descriptor":"lodash@npm:^4.17.21"}}} | ||
| {"value":"@sourceacademy/modules-repotools@virtual:420fa510eaf06edbb20ddb72fbcd454e937e7e266b8c6b7a55ab314e0562f2688175809f2b44c78563a0467da21b6d34688fe0020ecef5e669997fe518ee56e1#workspace:lib/repotools","children":{"lodash@npm:4.17.21":{"locator":"lodash@npm:4.17.21","descriptor":"lodash@npm:^4.17.21"}}} | ||
| {"value":"@sourceacademy/modules-repotools@virtual:72a684de9912e8b42da7a581a5478d78ad73e0a505830e61e9b2fbc990fcc10719f71ef5373c26313c28b9295f1381b0d95a4d0ab2ae6e33671c2ba25a5c1dc2#workspace:lib/repotools","children":{"lodash@npm:4.17.21":{"locator":"lodash@npm:4.17.21","descriptor":"lodash@npm:^4.17.21"}}} | ||
| {"value":"@sourceacademy/modules-repotools@workspace:lib/repotools","children":{"lodash@npm:4.17.21":{"locator":"lodash@npm:4.17.21","descriptor":"lodash@npm:^4.17.21"}}} | ||
| {"value":"@sourceacademy/modules@workspace:.","children":{"lodash@npm:4.17.21":{"locator":"lodash@npm:4.17.21","descriptor":"lodash@npm:^4.17.21"}}} | ||
| {"value":"archiver-utils@npm:5.0.2","children":{"lodash@npm:4.17.21":{"locator":"lodash@npm:4.17.21","descriptor":"lodash@npm:^4.17.15"}}} | ||
| {"value":"js-slang@npm:1.0.85","children":{"lodash@npm:4.17.21":{"locator":"lodash@npm:4.17.21","descriptor":"lodash@npm:^4.17.21"}}} | ||
| {"value":"source-academy-wabt@npm:1.1.3","children":{"lodash@npm:4.17.21":{"locator":"lodash@npm:4.17.21","descriptor":"lodash@npm:^4.17.21"}}} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,173 @@ | ||
| import fs from 'fs/promises'; | ||
| import pathlib from 'path'; | ||
| import * as core from '@actions/core'; | ||
| import { getExecOutput } from '@actions/exec'; | ||
| import memoize from 'lodash/memoize.js'; | ||
| import { extractPkgsFromYarnLockV2 } from 'snyk-nodejs-lockfile-parser'; | ||
| import { gitRoot } from './gitRoot.js'; | ||
|
|
||
| const packageNameRE = /^(.+)@.+$/; | ||
|
|
||
| /** | ||
| * Lockfile specifications come in the form of package_name@resolution, but | ||
| * we only want the package name. This function extracts that package name, | ||
| * accounting for the fact that package names might start with '@' | ||
| */ | ||
| export function extractPackageName(raw: string) { | ||
| const match = packageNameRE.exec(raw); | ||
| if (!match) { | ||
| throw new Error(`Invalid package name: ${raw}`); | ||
| } | ||
|
|
||
| return match[1]; | ||
| } | ||
|
|
||
| /** | ||
| * Parses and lockfile's contents and extracts all the different dependencies and | ||
| * versions | ||
| */ | ||
| function processLockFileText(contents: string) { | ||
| const lockFile = extractPkgsFromYarnLockV2(contents); | ||
| const mappings = new Set<string>(); | ||
| for (const [pkgSpecifier, { resolution }] of Object.entries(lockFile)) { | ||
| mappings.add(`${pkgSpecifier} -> ${resolution}`); | ||
| } | ||
| return mappings; | ||
| } | ||
|
|
||
| /** | ||
| * Retrieves the contents of the lockfile in the repo | ||
| */ | ||
| async function getCurrentLockFile() { | ||
| const lockFilePath = pathlib.join(gitRoot, 'yarn.lock'); | ||
| const contents = await fs.readFile(lockFilePath, 'utf-8'); | ||
| return processLockFileText(contents); | ||
| } | ||
|
|
||
| /** | ||
| * Retrieves the contents of the lockfile on the master branch | ||
| */ | ||
| async function getMasterLockFile() { | ||
| const { stdout, stderr, exitCode } = await getExecOutput( | ||
| 'git', | ||
| [ | ||
| '--no-pager', | ||
| 'show', | ||
| 'origin/master:yarn.lock' | ||
| ], | ||
| { silent: true } | ||
| ); | ||
|
|
||
| if (exitCode !== 0) { | ||
| core.error(stderr); | ||
| throw new Error('git show exited with non-zero error-code'); | ||
| } | ||
|
|
||
| return processLockFileText(stdout); | ||
| } | ||
|
|
||
| interface ResolutionSpec { pkgSpecifier: string, pkgName: string } | ||
|
|
||
| /** | ||
| * Parsed output entry returned by `yarn why` | ||
| */ | ||
| interface YarnWhyOutput { | ||
| value: string; | ||
| children: { | ||
| [locator: string]: { | ||
| locator: string; | ||
| descriptor: string; | ||
| }; | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Run `yarn why <package_name>` to see why a package is included | ||
| * Don't use recursive (-R) since we want to build the graph ourselves | ||
| * @function | ||
| */ | ||
| const runYarnWhy = memoize(async (pkgName: string) => { | ||
| // Memoize the call so that we don't need to call yarn why multiple times for each package | ||
| const { stdout: output, exitCode, stderr } = await getExecOutput('yarn', ['why', pkgName, '--json'], { silent: true }); | ||
| if (exitCode !== 0) { | ||
| core.error(stderr); | ||
| throw new Error(`yarn why for ${pkgName} failed!`); | ||
| } | ||
|
|
||
| return output.split('\n').reduce<YarnWhyOutput[]>((res, each) => { | ||
| each = each.trim(); | ||
| if (each === '') return res; | ||
|
|
||
| const pkg = JSON.parse(each) as YarnWhyOutput; | ||
| return [...res, pkg]; | ||
| }, []); | ||
| }) | ||
|
|
||
| /** | ||
| * Determines the names of the packages that have changed versions | ||
| */ | ||
| export async function getPackagesWithResolutionChanges() { | ||
| const [currentLockFileMappings, masterLockFileMappings] = await Promise.all([ | ||
| getCurrentLockFile(), | ||
| getMasterLockFile() | ||
| ]); | ||
|
|
||
| const changes = new Set(masterLockFileMappings); | ||
| for (const edge of currentLockFileMappings) { | ||
| changes.delete(edge); | ||
| } | ||
|
|
||
| const frontier: ResolutionSpec[] = []; | ||
| const changedDeps = new Set<string>(); | ||
| for (const edge of changes) { | ||
| const [pkgSpecifier] = edge.split(' -> '); | ||
| changedDeps.add(pkgSpecifier); | ||
| frontier.push({ | ||
| pkgSpecifier, | ||
| pkgName: extractPackageName(pkgSpecifier) | ||
| }); | ||
| } | ||
|
|
||
| while (frontier.length > 0) { | ||
| const { pkgName, pkgSpecifier } = frontier.shift()!; | ||
|
|
||
| const reasons = await runYarnWhy(pkgName); | ||
| reasons.forEach(pkg => { | ||
| if (changedDeps.has(pkg.value)) { | ||
| // If we've already added this pkg specifier, don't need to explore it again | ||
| return; | ||
| } | ||
|
|
||
| const childrenSpecifiers = Object.values(pkg.children).map(({ descriptor }) => descriptor); | ||
| if (!childrenSpecifiers.includes(pkgSpecifier)) return; | ||
|
|
||
| frontier.push({ pkgSpecifier: pkg.value, pkgName: extractPackageName(pkg.value) }); | ||
| changedDeps.add(pkg.value); | ||
| }); | ||
| } | ||
|
|
||
| core.info('=== Summary of dirty monorepo packages ===\n'); | ||
| const pkgsToRebuild = [...changedDeps].filter(pkgSpecifier => pkgSpecifier.includes('@workspace:')); | ||
| for (const pkgName of pkgsToRebuild) { | ||
| core.info(`- ${pkgName}`); | ||
| } | ||
|
|
||
| return pkgsToRebuild.map(extractPackageName); | ||
| } | ||
|
|
||
| /** | ||
| * Returns `true` if there are changes present in the given directory relative to | ||
| * the master branch\ | ||
| * Used to determine, particularly for libraries, if running tests and tsc are necessary | ||
| */ | ||
| export const hasLockFileChanged = memoize(async () => { | ||
| const { exitCode } = await getExecOutput( | ||
| 'git --no-pager diff --quiet origin/master -- yarn.lock', | ||
| [], | ||
| { | ||
| failOnStdErr: false, | ||
| ignoreReturnCode: true | ||
| } | ||
| ); | ||
| return exitCode !== 0; | ||
| }); | ||
109 changes: 0 additions & 109 deletions
109
.github/actions/src/lockfiles/__tests__/lockfiles.test.ts
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug:
getExecOutputis called with the full command string as the executable, instead of separating the command and its arguments.Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The
getExecOutputfunction is called with the entire commandgit --no-pager diff --quiet origin/master -- yarn.lockas a single string for thecommandparameter. The@actions/execAPI expects the executable and its arguments to be separate parameters. This will cause the shell to attempt to execute a non-existent command named"git --no-pager diff --quiet origin/master -- yarn.lock", leading to a failure when checking lockfile changes.💡 Suggested Fix
Pass
'git'as the first argument and['--no-pager', 'diff', '--quiet', 'origin/master', '--', 'yarn.lock']as the second argument togetExecOutput.🤖 Prompt for AI Agent
Did we get this right? 👍 / 👎 to inform future reviews.