-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add bazel-deps-query config to dynamically resolve additional paths #13
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
base: main
Are you sure you want to change the base?
Changes from all commits
49b7389
8cdd609
555a901
b64e18d
54b3a81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,7 @@ import { | |
| } from './util/pull-request-overflow-handler'; | ||
| import {signoffCommitMessage} from './util/signoff-commit-message'; | ||
| import {CommitExclude} from './util/commit-exclude'; | ||
| import {runBazelQuery, resolveBazelQuery} from './util/bazel-query'; | ||
|
|
||
| type ExtraGenericFile = { | ||
| type: 'generic'; | ||
|
|
@@ -140,6 +141,7 @@ export interface ReleaserConfig { | |
| // Manifest only | ||
| excludePaths?: string[]; | ||
| additionalPaths?: string[]; | ||
| bazelDepsQuery?: boolean | string; | ||
| } | ||
|
|
||
| export interface CandidateReleasePullRequest { | ||
|
|
@@ -188,6 +190,7 @@ interface ReleaserConfigJson { | |
| 'initial-version'?: string; | ||
| 'exclude-paths'?: string[]; // manifest-only | ||
| 'additional-paths'?: string[]; // manifest-only | ||
| 'bazel-deps-query'?: boolean | string; // manifest-only | ||
| 'date-format'?: string; | ||
| } | ||
|
|
||
|
|
@@ -670,14 +673,33 @@ export class Manifest { | |
| ); | ||
| } | ||
|
|
||
| // resolve bazel-deps-query for each package and merge with additionalPaths | ||
| const resolvedAdditionalPaths: Record<string, string[]> = {}; | ||
| for (const [path, config] of Object.entries(this.repositoryConfig)) { | ||
| const staticPaths = config.additionalPaths || []; | ||
| let bazelPaths: string[] = []; | ||
| if (config.bazelDepsQuery) { | ||
| const queryExpression = resolveBazelQuery(config.bazelDepsQuery, path); | ||
| bazelPaths = runBazelQuery(queryExpression, path, this.logger); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @devin can you debug log the bazel paths
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @devin can you debug log the bazel paths There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added debug logging for the resolved paths in 555a901:
So you should see the resolved paths when debug logging is enabled.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @devin actually switch all those logs to info level There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 54b3a81: switched the bazel-deps-query logs in both |
||
| this.logger.info( | ||
| `bazel-deps-query resolved paths for ${path}: ${JSON.stringify( | ||
| bazelPaths | ||
| )}` | ||
| ); | ||
| } | ||
| // merge and deduplicate | ||
| const allPaths = [...new Set([...staticPaths, ...bazelPaths])]; | ||
| resolvedAdditionalPaths[path] = allPaths; | ||
| } | ||
|
|
||
| // split commits by path | ||
| this.logger.info(`Splitting ${commits.length} commits by path`); | ||
| const cs = new CommitSplit({ | ||
| includeEmpty: true, | ||
| packagePaths: Object.fromEntries( | ||
| Object.entries(this.repositoryConfig).map(([path, config]) => [ | ||
| Object.entries(this.repositoryConfig).map(([path]) => [ | ||
| path, | ||
| config.additionalPaths || [], | ||
| resolvedAdditionalPaths[path] || [], | ||
| ]) | ||
| ), | ||
| }); | ||
|
|
@@ -1414,6 +1436,7 @@ function extractReleaserConfig( | |
| initialVersion: config['initial-version'], | ||
| excludePaths: config['exclude-paths'], | ||
| additionalPaths: config['additional-paths'], | ||
| bazelDepsQuery: config['bazel-deps-query'], | ||
| dateFormat: config['date-format'], | ||
| }; | ||
| } | ||
|
|
@@ -1776,6 +1799,7 @@ function mergeReleaserConfig( | |
| excludePaths: pathConfig.excludePaths ?? defaultConfig.excludePaths, | ||
| additionalPaths: | ||
| pathConfig.additionalPaths ?? defaultConfig.additionalPaths, | ||
| bazelDepsQuery: pathConfig.bazelDepsQuery ?? defaultConfig.bazelDepsQuery, | ||
| dateFormat: pathConfig.dateFormat ?? defaultConfig.dateFormat, | ||
| }; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,175 @@ | ||||||||||||||||||||||||||||
| // Copyright 2024 Google LLC | ||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||||||||||||||||||||||
| // you may not use this file except in compliance with the License. | ||||||||||||||||||||||||||||
| // You may obtain a copy of the License at | ||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||
| // Unless required by applicable law or agreed to in writing, software | ||||||||||||||||||||||||||||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||||||||||||||||||||||||||||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||||||||||||||||||||||||
| // See the License for the specific language governing permissions and | ||||||||||||||||||||||||||||
| // limitations under the License. | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| import {execFileSync} from 'child_process'; | ||||||||||||||||||||||||||||
| import {Logger} from './logger'; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Parse the output of a bazel query command into a list of local package paths. | ||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||
| * Bazel query output contains lines like: | ||||||||||||||||||||||||||||
| * //path/to/package:target_name | ||||||||||||||||||||||||||||
| * @external_dep//path:target | ||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||
| * This function: | ||||||||||||||||||||||||||||
| * 1. Filters out external dependencies (lines starting with `@`) | ||||||||||||||||||||||||||||
| * 2. Extracts the package path from local targets (`//path/to/package:target` -> `path/to/package`) | ||||||||||||||||||||||||||||
| * 3. Deduplicates paths | ||||||||||||||||||||||||||||
| * 4. Optionally excludes a given path (e.g., the package's own path) | ||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||
| * @param output Raw stdout from a bazel query command | ||||||||||||||||||||||||||||
| * @param excludePath Optional path to exclude from results (e.g., the package's own path) | ||||||||||||||||||||||||||||
| * @returns Array of unique local package paths | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| export function parseBazelQueryOutput( | ||||||||||||||||||||||||||||
| output: string, | ||||||||||||||||||||||||||||
| excludePath?: string | ||||||||||||||||||||||||||||
| ): string[] { | ||||||||||||||||||||||||||||
| const lines = output.split('\n').filter(line => line.trim().length > 0); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const paths = new Set<string>(); | ||||||||||||||||||||||||||||
| for (const line of lines) { | ||||||||||||||||||||||||||||
| const trimmed = line.trim(); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Skip external dependencies (start with @) | ||||||||||||||||||||||||||||
| if (trimmed.startsWith('@')) { | ||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Match local targets: //path/to/package:target or //path/to/package | ||||||||||||||||||||||||||||
| const match = trimmed.match(/^\/\/([^:]*)/); | ||||||||||||||||||||||||||||
| if (!match) { | ||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const packagePath = match[1]; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Skip empty paths (e.g., //:target refers to the root) | ||||||||||||||||||||||||||||
| if (!packagePath) { | ||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Normalize: remove trailing slashes | ||||||||||||||||||||||||||||
| const normalized = packagePath.replace(/\/+$/, ''); | ||||||||||||||||||||||||||||
| if (!normalized) { | ||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Exclude the package's own path if specified | ||||||||||||||||||||||||||||
| if (excludePath && normalized === excludePath.replace(/\/+$/, '')) { | ||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| paths.add(normalized); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return Array.from(paths).sort(); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Build the default bazel query expression for a given package path. | ||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||
| * @param packagePath The package path (e.g., "apps/my-app") | ||||||||||||||||||||||||||||
| * @returns The bazel query expression (passed to `bazel query`) | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| export function buildBazelQueryExpression(packagePath: string): string { | ||||||||||||||||||||||||||||
| return `deps(//${packagePath})`; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Resolve the bazel query expression from the config value. | ||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||
| * - If the config value is `true`, build the default expression from the package path. | ||||||||||||||||||||||||||||
| * - If the config value is a string: | ||||||||||||||||||||||||||||
| * - If it starts with `bazel query`, treat it as a full command and extract the query expression | ||||||||||||||||||||||||||||
| * - Otherwise, treat it as a query expression directly | ||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||
| * @param configValue The `bazel-deps-query` config value (boolean or string) | ||||||||||||||||||||||||||||
| * @param packagePath The package path from the config key | ||||||||||||||||||||||||||||
| * @returns The resolved bazel query expression | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| export function resolveBazelQuery( | ||||||||||||||||||||||||||||
| configValue: boolean | string, | ||||||||||||||||||||||||||||
| packagePath: string | ||||||||||||||||||||||||||||
| ): string { | ||||||||||||||||||||||||||||
| if (configValue === true) { | ||||||||||||||||||||||||||||
| return buildBazelQueryExpression(packagePath); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // A boolean `false` behaves like "disabled". | ||||||||||||||||||||||||||||
| if (configValue === false) { | ||||||||||||||||||||||||||||
| return ''; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const trimmed = configValue.trim(); | ||||||||||||||||||||||||||||
| const bazelQueryPrefix = /^bazel\s+query\s+/; | ||||||||||||||||||||||||||||
| if (bazelQueryPrefix.test(trimmed)) { | ||||||||||||||||||||||||||||
| let expr = trimmed.replace(bazelQueryPrefix, '').trim(); | ||||||||||||||||||||||||||||
| // If the expression is quoted, strip surrounding quotes. | ||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||
| (expr.startsWith('"') && expr.endsWith('"')) || | ||||||||||||||||||||||||||||
| (expr.startsWith("'") && expr.endsWith("'")) | ||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||
| expr = expr.slice(1, -1); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+119
to
+125
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Quote stripping can corrupt multi-part expressions The quote-stripping logic checks if the expression starts and ends with the same quote character, then strips both. This will silently corrupt valid multi-argument bazel query inputs. For example, A safer approach would be to only strip quotes when the inner content contains no unescaped quotes of the same type:
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/util/bazel-query.ts
Line: 119-125
Comment:
**Quote stripping can corrupt multi-part expressions**
The quote-stripping logic checks if the expression starts and ends with the same quote character, then strips both. This will silently corrupt valid multi-argument bazel query inputs.
For example, `bazel query 'deps(//a)' union 'deps(//b)'` after prefix removal yields `'deps(//a)' union 'deps(//b)'`. This starts with `'` and ends with `'`, so the code strips them, producing `deps(//a)' union 'deps(//b)` — a broken expression.
A safer approach would be to only strip quotes when the inner content contains no unescaped quotes of the same type:
```suggestion
if (
(expr.startsWith('"') && expr.endsWith('"') && !expr.slice(1, -1).includes('"')) ||
(expr.startsWith("'") && expr.endsWith("'") && !expr.slice(1, -1).includes("'"))
) {
expr = expr.slice(1, -1);
}
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||
| return expr; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return trimmed; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Execute a bazel query expression and return the parsed local package paths. | ||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||
| * This uses `execFileSync` (no shell) to reduce the risk of command injection. | ||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||
| * @param queryExpression The query expression to execute (e.g., "deps(//combined-service)") | ||||||||||||||||||||||||||||
| * @param excludePath Optional path to exclude from results | ||||||||||||||||||||||||||||
| * @param logger Optional logger instance | ||||||||||||||||||||||||||||
| * @returns Array of unique local package paths | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| export function runBazelQuery( | ||||||||||||||||||||||||||||
| queryExpression: string, | ||||||||||||||||||||||||||||
| excludePath?: string, | ||||||||||||||||||||||||||||
| logger?: Logger | ||||||||||||||||||||||||||||
| ): string[] { | ||||||||||||||||||||||||||||
| logger?.info(`Running bazel deps query: bazel query '${queryExpression}'`); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||
| const output = execFileSync('bazel', ['query', queryExpression], { | ||||||||||||||||||||||||||||
| encoding: 'utf-8', | ||||||||||||||||||||||||||||
| timeout: 120000, // 2 minute timeout | ||||||||||||||||||||||||||||
| stdio: ['pipe', 'pipe', 'pipe'], | ||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const paths = parseBazelQueryOutput(output, excludePath); | ||||||||||||||||||||||||||||
| logger?.info( | ||||||||||||||||||||||||||||
| `Bazel deps query resolved ${ | ||||||||||||||||||||||||||||
| paths.length | ||||||||||||||||||||||||||||
| } additional paths: ${JSON.stringify(paths)}` | ||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||
| return paths; | ||||||||||||||||||||||||||||
| } catch (err) { | ||||||||||||||||||||||||||||
| const error = err as Error & {stderr?: string}; | ||||||||||||||||||||||||||||
| logger?.error( | ||||||||||||||||||||||||||||
| `Failed to run bazel deps query "${queryExpression}": ${error.message}` | ||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||
| if (error.stderr) { | ||||||||||||||||||||||||||||
| logger?.error(`stderr: ${error.stderr}`); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| throw new Error( | ||||||||||||||||||||||||||||
| `Failed to execute bazel-deps-query "${queryExpression}": ${error.message}` | ||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| { | ||
| "release-type": "simple", | ||
| "packages": { | ||
| "apps/my-app": { | ||
| "bazel-deps-query": true | ||
| } | ||
| } | ||
| } |
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.
Empty query expression passed to
runBazelQueryWhen
config.bazelDepsQueryis a non-empty string (truthy),resolveBazelQuerycan still return an empty string — for example, if the config value is"bazel query "(just the prefix with trailing whitespace). In that case,runBazelQuery("")would be called, executingbazel query ""which will fail with a confusing error.Consider guarding against an empty resolved expression:
Prompt To Fix With AI