diff --git a/.github/workflows/version_scanner.yml b/.github/workflows/version_scanner.yml index 52f813e67995..078e4259e491 100644 --- a/.github/workflows/version_scanner.yml +++ b/.github/workflows/version_scanner.yml @@ -35,7 +35,7 @@ jobs: # Uses -o to output a detailed, raw CSV to a file # Uses --stdout to print a slim, easier to parse summary to the GitHub Actions UI # Uses --soft-fail to temporarily limit causing CI/CD failures during the migration to full operation. - python scripts/version_scanner/version_scanner.py -d python -v 3.7 --stdout -o version_scanner_output.csv --soft-fail + python scripts/version_scanner/version_scanner.py --matrix-file scripts/version_scanner/matrix.yaml --package-file scripts/version_scanner/example-list-non-generated-packages.txt --stdout -o version_scanner_output.csv --soft-fail - name: Upload CSV Results if: always() diff --git a/scripts/version_scanner/example-list-non-generated-packages.txt b/scripts/version_scanner/example-list-non-generated-packages.txt new file mode 100644 index 000000000000..bfc1e3fe8658 --- /dev/null +++ b/scripts/version_scanner/example-list-non-generated-packages.txt @@ -0,0 +1,31 @@ +packages/bigframes +packages/bigquery-magics +packages/db-dtypes +packages/django-google-spanner +packages/gapic-generator +packages/google-api-core +# packages/google-api-python-client # non-monorepo, ignore for now. +packages/google-auth +packages/google-auth-httplib2 +packages/google-auth-oauthlib +packages/google-cloud-bigquery +packages/pandas-gbq +packages/google-cloud-bigtable +packages/google-cloud-core +packages/google-crc32c +packages/google-cloud-datastore +packages/google-cloud-dns +packages/google-cloud-documentai-toolbox +packages/google-cloud-error-reporting +packages/google-cloud-firestore +packages/google-cloud-logging +packages/google-cloud-ndb +packages/google-cloud-pubsub +packages/google-cloud-runtimeconfig +packages/google-cloud-spanner +packages/google-cloud-storage +packages/google-cloud-testutils +packages/google-resumable-media +packages/proto-plus +packages/sqlalchemy-bigquery +packages/sqlalchemy-spanner diff --git a/scripts/version_scanner/matrix.yaml b/scripts/version_scanner/matrix.yaml new file mode 100644 index 000000000000..0f50b31fd48f --- /dev/null +++ b/scripts/version_scanner/matrix.yaml @@ -0,0 +1,4 @@ +python: + - "3.7" + - "3.8" + - "3.9" diff --git a/scripts/version_scanner/small_package_list.txt b/scripts/version_scanner/small_package_list.txt deleted file mode 100644 index 8c9a4f39e879..000000000000 --- a/scripts/version_scanner/small_package_list.txt +++ /dev/null @@ -1,6 +0,0 @@ -# Example package list for filtering scanning targets via the --package-file option. -packages/google-cloud-access-context-manager -packages/google-cloud-bigtable -packages/google-cloud-biglake-hive -packages/google-cloud-documentai-toolbox -packages/google-cloud-core diff --git a/scripts/version_scanner/tests/unit/test_version_scanner.py b/scripts/version_scanner/tests/unit/test_version_scanner.py index 054df22421bc..a151c1c55e11 100644 --- a/scripts/version_scanner/tests/unit/test_version_scanner.py +++ b/scripts/version_scanner/tests/unit/test_version_scanner.py @@ -59,6 +59,8 @@ def sample_match(): (PermissionError(), False, False, False, "Warning: Permission denied reading test_desc", None), # Optional PermissionError (IOError("disk full"), True, False, True, "Error reading test_desc", None), # Required IOError (IOError("disk full"), False, False, False, "Warning: Error reading test_desc", None), # Optional IOError + (ValueError("invalid bytes"), True, False, True, "Error reading test_desc", None), # Required ValueError + (ValueError("invalid bytes"), False, False, False, "Warning: Error reading test_desc", None), # Optional ValueError ] ) def test_safe_read_file_scenarios( @@ -782,16 +784,16 @@ def test_format_for_console(sample_match): assert "python_requires = " not in log_str # Slim format doesn't print context line -def test_parse_targets_file(tmp_path): - from version_scanner import parse_targets_file - yaml_file = tmp_path / "targets.yaml" +def test_parse_matrix_file(tmp_path): + from version_scanner import parse_matrix_file + yaml_file = tmp_path / "matrix.yaml" yaml_file.write_text(""" python: - "3.7" - "3.8" protobuf: "4.25.8" """) - targets = parse_targets_file(str(yaml_file)) + targets = parse_matrix_file(str(yaml_file)) assert targets == [("python", "3.7"), ("python", "3.8"), ("protobuf", "4.25.8")] @pytest.mark.parametrize( @@ -801,20 +803,22 @@ def test_parse_targets_file(tmp_path): ("invalid: {", True), # Invalid YAML ("- not_a_mapping", True), # Invalid structure (list instead of map) ("python:\n - null", True), # Invalid version type (null/None value) + ("python:\n - 3.10", True), # Invalid version type (float instead of string in list) + ("python: 3.10", True), # Invalid version type (float instead of string) ] ) -def test_parse_targets_file_failures(tmp_path, file_content, file_exists): - from version_scanner import parse_targets_file +def test_parse_matrix_file_failures(tmp_path, file_content, file_exists): + from version_scanner import parse_matrix_file if file_exists: - yaml_file = tmp_path / "targets_failures.yaml" + yaml_file = tmp_path / "matrix_failures.yaml" yaml_file.write_text(file_content) path = str(yaml_file) else: path = "nonexistent_file.yaml" with pytest.raises(SystemExit) as excinfo: - parse_targets_file(path) + parse_matrix_file(path) assert excinfo.value.code == 1 def test_scan_repository_multi_targets(tmp_path): @@ -868,3 +872,26 @@ def test_scan_repository_multi_targets(tmp_path): assert protobuf_match[0]["version"] == "4.25.8" assert protobuf_match[0]["rule_name"] == "protobuf_check" + +@pytest.mark.parametrize( + "args, expected_error_msg", + [ + # Mixing -m/--matrix-file with -d or -v + (['version_scanner.py', '-m', 'matrix.yaml', '-d', 'python'], "Cannot specify -d/--dependency or -v/--version when using -m/--matrix-file"), + (['version_scanner.py', '-m', 'matrix.yaml', '-v', '3.7'], "Cannot specify -d/--dependency or -v/--version when using -m/--matrix-file"), + (['version_scanner.py', '-m', 'matrix.yaml', '-d', 'python', '-v', '3.7'], "Cannot specify -d/--dependency or -v/--version when using -m/--matrix-file"), + # Missing either -d or -v when not using -m + (['version_scanner.py', '-d', 'python'], "Must specify both -d/--dependency and -v/--version when not using -m/--matrix-file"), + (['version_scanner.py', '-v', '3.7'], "Must specify both -d/--dependency and -v/--version when not using -m/--matrix-file"), + (['version_scanner.py'], "Must specify both -d/--dependency and -v/--version when not using -m/--matrix-file"), + ] +) +def test_main_cli_validation(capsys, args, expected_error_msg): + from version_scanner import main + with mock.patch('sys.argv', args): + with pytest.raises(SystemExit) as excinfo: + main() + assert excinfo.value.code == 2 + captured = capsys.readouterr() + assert expected_error_msg in captured.err + diff --git a/scripts/version_scanner/version_scanner.py b/scripts/version_scanner/version_scanner.py index 6205e8effadd..fcd45ed63b54 100644 --- a/scripts/version_scanner/version_scanner.py +++ b/scripts/version_scanner/version_scanner.py @@ -65,7 +65,7 @@ def _safe_read_file( else: print(f"Warning: Permission denied reading {description}: {file_path}", file=sys.stderr) return None - except IOError as e: + except (IOError, ValueError) as e: if required: print(f"Error reading {description} {file_path}: {e}", file=sys.stderr) sys.exit(1) @@ -624,33 +624,36 @@ def scan_repository( return results -def parse_targets_file(file_path: str) -> List[Tuple[str, str]]: +def parse_matrix_file(file_path: str) -> List[Tuple[str, str]]: """ - Parses a YAML targets file into a list of (dependency, version) tuples. + Parses a YAML matrix file into a list of (dependency, version) tuples. """ - content = _safe_read_file(file_path, required=True, description="targets file") + content = _safe_read_file(file_path, required=True, description="matrix file") try: - raw_targets = yaml.safe_load(content) + raw_matrix = yaml.safe_load(content) except Exception as e: - print(f"Error parsing targets YAML mapping: {e}", file=sys.stderr) + print(f"Error parsing matrix YAML mapping: {e}", file=sys.stderr) sys.exit(1) - if not isinstance(raw_targets, dict): - print("Error: Targets file content must resolve to a YAML mapping", file=sys.stderr) + if not isinstance(raw_matrix, dict): + print("Error: Matrix file content must resolve to a YAML mapping", file=sys.stderr) sys.exit(1) targets = [] - for dep, versions in raw_targets.items(): + for dep, versions in raw_matrix.items(): if isinstance(versions, list): for v in versions: if v is None or isinstance(v, (dict, list)): print(f"Error: Invalid version '{v}' for dependency '{dep}'", file=sys.stderr) sys.exit(1) - targets.append((str(dep), str(v))) - elif versions is not None and not isinstance(versions, dict): - targets.append((str(dep), str(versions))) + if not isinstance(v, str): + print(f"Error: Version '{v}' for dependency '{dep}' must be specified as a quoted string to prevent YAML parsing issues (e.g., 3.10 parsed as 3.1).", file=sys.stderr) + sys.exit(1) + targets.append((str(dep), v)) + elif isinstance(versions, str): + targets.append((str(dep), versions)) else: - print(f"Error: Invalid version '{versions}' for dependency '{dep}'", file=sys.stderr) + print(f"Error: Invalid version '{versions}' for dependency '{dep}'. Versions must be specified as quoted strings.", file=sys.stderr) sys.exit(1) return targets @@ -675,7 +678,7 @@ def main(): ) parser.add_argument( - "--targets-file", + "-m", "--matrix-file", help="Path to a YAML file containing target dependencies and versions." ) @@ -743,17 +746,17 @@ def main(): args = parser.parse_args() # Validation of required inputs - has_single_target = bool(args.dependency and args.version) - has_targets_file = bool(args.targets_file) - - if not (has_single_target or has_targets_file): - parser.error("Must specify either (-d/--dependency AND -v/--version) OR (--targets-file)") - if has_single_target and has_targets_file: - parser.error("Cannot specify both single target (-d/-v) and targets file (--targets-file)") + has_matrix_file = bool(args.matrix_file) + if has_matrix_file: + if args.dependency or args.version: + parser.error("Cannot specify -d/--dependency or -v/--version when using -m/--matrix-file") + else: + if not (args.dependency and args.version): + parser.error("Must specify both -d/--dependency and -v/--version when not using -m/--matrix-file") targets = [] - if has_targets_file: - targets = parse_targets_file(args.targets_file) + if has_matrix_file: + targets = parse_matrix_file(args.matrix_file) else: targets = [(args.dependency, args.version)] @@ -772,7 +775,7 @@ def main(): elif args.package_file: target_packages = read_package_file(args.package_file) - if has_targets_file: + if has_matrix_file: print("Starting scan for multiple targets:") for dep, ver in targets: print(f" - {dep}: {ver}") @@ -809,7 +812,7 @@ def main(): rules, target_packages, ignore_dirs, - version_string=(None if has_targets_file else args.version), + version_string=(None if has_matrix_file else args.version), targets=targets ) @@ -833,8 +836,8 @@ def main(): script_dir = os.path.dirname(os.path.abspath(__file__)) results_dir = os.path.join(script_dir, "results") os.makedirs(results_dir, exist_ok=True) - if has_targets_file: - base_name = os.path.splitext(os.path.basename(args.targets_file))[0] + if has_matrix_file: + base_name = os.path.splitext(os.path.basename(args.matrix_file))[0] output_path = os.path.join(results_dir, f"{base_name}-{timestamp}.csv") else: output_path = os.path.join(results_dir, f"{args.dependency}-{args.version}-{timestamp}.csv")