Skip to content

Commit 889db0a

Browse files
l0lawrenceCopilot
andcommitted
Refactor changelog package resolution to use get_targeted_directories
Per review feedback on #46016, route package discovery through the shared Check.get_targeted_directories pipeline used by import_all and other azpysdk checks instead of custom discover_targeted_packages / walk-up logic. Behavior changes: - add/create/status now inherit the common 'target' positional (default '**'). - Subdirectory walk-up is dropped: run from package root with '.' or pass the package path/name explicitly. - verify is unchanged (always repo-level, no target). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 593b23b commit 889db0a

2 files changed

Lines changed: 153 additions & 240 deletions

File tree

eng/tools/azure-sdk-tools/azpysdk/changelog.py

Lines changed: 57 additions & 112 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,7 @@
66
from typing import Optional, List
77

88
from .Check import Check, REPO_ROOT
9-
from ci_tools.functions import discover_targeted_packages
109
from ci_tools.logging import logger
11-
from ci_tools.parsing import ParsedSetup
1210

1311
# The expected Chronus package name and on-disk install location.
1412
# Chronus is pinned as a dev dependency in .github/chronus/package.json with
@@ -63,10 +61,14 @@ def register(
6361
) -> None:
6462
"""Register the ``changelog`` command group.
6563
66-
The *parent_parsers* (common args like ``target``, ``--isolate``) are
67-
intentionally **not** used here because changelog commands operate at
68-
the repository level via Chronus, not on individual packages.
64+
``add``, ``create``, and ``status`` inherit the shared
65+
``target`` positional (and related common args) from
66+
*parent_parsers* so that package resolution goes through the same
67+
:meth:`Check.get_targeted_directories` pipeline used by
68+
``import_all`` and the other ``azpysdk`` checks. ``verify``
69+
always operates at the repo level, so it does not take a target.
6970
"""
71+
parents = parent_parsers or []
7072
p = subparsers.add_parser(
7173
"changelog",
7274
help="Manage changelogs with Chronus (add, verify, create, status)",
@@ -76,16 +78,8 @@ def register(
7678
changelog_sub = p.add_subparsers(title="changelog commands", dest="changelog_command")
7779

7880
# changelog add
79-
add_p = changelog_sub.add_parser("add", help="Add a chronus change entry for modified packages")
80-
add_p.add_argument(
81-
"package",
82-
nargs="?",
83-
default=None,
84-
help=(
85-
"Package path (e.g. sdk/storage/azure-storage-blob) to add an entry for. "
86-
"If omitted and CWD is inside a package directory, the package is detected "
87-
"automatically. Otherwise chronus detects modified packages interactively."
88-
),
81+
add_p = changelog_sub.add_parser(
82+
"add", parents=parents, help="Add a chronus change entry for modified packages"
8983
)
9084
add_p.add_argument(
9185
"--kind",
@@ -102,37 +96,19 @@ def register(
10296
)
10397
add_p.set_defaults(func=self._run_add)
10498

105-
# changelog verify
99+
# changelog verify (operates on the whole repo; no target)
106100
verify_p = changelog_sub.add_parser("verify", help="Verify all modified packages have change entries")
107101
verify_p.set_defaults(func=self._run_verify)
108102

109103
# changelog create
110-
create_p = changelog_sub.add_parser("create", help="Generate CHANGELOG.md from pending chronus entries")
111-
create_p.add_argument(
112-
"package",
113-
nargs="?",
114-
default=None,
115-
help=(
116-
"Package path (e.g. sdk/storage/azure-storage-blob) to generate changelog for. "
117-
"If omitted and CWD is inside a package directory, the package is detected "
118-
"automatically. Otherwise chronus generates changelogs for all packages."
119-
),
104+
create_p = changelog_sub.add_parser(
105+
"create", parents=parents, help="Generate CHANGELOG.md from pending chronus entries"
120106
)
121107
create_p.set_defaults(func=self._run_create)
122108

123109
# changelog status
124110
status_p = changelog_sub.add_parser(
125-
"status", help="Show a summary of pending changes and resulting version bumps"
126-
)
127-
status_p.add_argument(
128-
"package",
129-
nargs="?",
130-
default=None,
131-
help=(
132-
"Package path (e.g. sdk/storage/azure-storage-blob) to show status for. "
133-
"If omitted and CWD is inside a package directory, the package is detected "
134-
"automatically. Otherwise chronus shows status for all packages."
135-
),
111+
"status", parents=parents, help="Show a summary of pending changes and resulting version bumps"
136112
)
137113
status_p.set_defaults(func=self._run_status)
138114

@@ -213,58 +189,32 @@ def _ensure_chronus_installed(self) -> None:
213189
)
214190
raise SystemExit(1)
215191

216-
def _find_package_root_from_cwd(self) -> Optional[str]:
217-
"""Find the package root directory when CWD is at or below ``sdk/<service>/<package>``.
218-
219-
Walks up from the current directory to locate the package root,
220-
unlike ``get_targeted_directories(target=".")`` which only works
221-
when CWD is exactly the package root. This lets developers run
222-
changelog commands from subdirectories such as
223-
``sdk/core/azure-core/tests/``.
224-
225-
Returns the absolute path to the package root, or ``None`` if CWD
226-
is not inside an ``sdk/<service>/<package>`` tree.
227-
"""
228-
try:
229-
cwd = os.path.abspath(os.getcwd())
230-
repo = os.path.abspath(REPO_ROOT)
231-
rel = os.path.relpath(cwd, repo)
232-
except ValueError:
233-
# On Windows, relpath raises ValueError when paths are on different drives
234-
return None
192+
def _resolve_package(self, args: argparse.Namespace) -> Optional[str]:
193+
"""Resolve ``args`` to a single Chronus package name.
235194
236-
parts = rel.replace("\\", "/").split("/")
237-
if len(parts) >= 3 and parts[0] == "sdk":
238-
return os.path.join(repo, parts[0], parts[1], parts[2])
239-
return None
195+
Delegates to :meth:`Check.get_targeted_directories` — the same
196+
discovery pipeline used by ``import_all`` and the other
197+
``azpysdk`` checks. The first discovered package's name is
198+
returned (matching chronus's single-package model).
240199
241-
def _resolve_package(self, package_arg: Optional[str]) -> Optional[str]:
242-
"""Resolve a package argument or CWD to a Chronus package name.
200+
Returns ``None`` when:
243201
244-
Uses ``discover_targeted_packages`` the same discovery function
245-
that powers ``get_targeted_directories`` — to locate packages by
246-
path or bare name. When *package_arg* is ``None``, the method
247-
detects the package from CWD by walking up to the nearest
248-
``sdk/<service>/<package>`` directory.
202+
* ``args.target`` is unset or left at the default glob ``"**"``
203+
— the user hasn't scoped to a package, so chronus runs without
204+
a package argument (interactive / whole-repo mode).
205+
* No package is discovered for the provided target (e.g.
206+
``target="."`` from a directory that isn't a package root).
249207
"""
250-
if package_arg:
251-
found = discover_targeted_packages(package_arg, REPO_ROOT)
252-
if found:
253-
try:
254-
return ParsedSetup.from_path(found[0]).name
255-
except Exception:
256-
return os.path.basename(found[0])
257-
# Not found by discovery — pass through as-is
258-
return package_arg
259-
260-
# No explicit package — detect from CWD
261-
pkg_root = self._find_package_root_from_cwd()
262-
if pkg_root is None:
208+
target = getattr(args, "target", None)
209+
if not target or target == "**":
263210
return None
264211
try:
265-
return ParsedSetup.from_path(pkg_root).name
212+
targeted = self.get_targeted_directories(args)
266213
except Exception:
267-
return os.path.basename(os.path.normpath(pkg_root))
214+
return None
215+
if not targeted:
216+
return None
217+
return targeted[0].name
268218

269219
def _run_chronus(self, chronus_args: List[str]) -> int:
270220
"""Run a chronus command from the repository root.
@@ -286,21 +236,21 @@ def _run_chronus(self, chronus_args: List[str]) -> int:
286236
# ------------------------------------------------------------------
287237

288238
def _run_add(self, args: argparse.Namespace) -> int:
289-
"""Run ``chronus add`` to interactively add a change entry.
290-
291-
When no *package* argument is given but CWD is inside a package
292-
directory (``sdk/<service>/<package>``), the package path is detected
293-
automatically so the developer doesn't have to specify it.
294-
295-
Optional ``--kind`` and ``--message`` flags are forwarded to chronus
296-
so the developer can skip the interactive prompts (e.g.
297-
``azpysdk changelog add --kind breaking -m "Removed foo API"``).
239+
"""Run ``chronus add`` to add a change entry.
240+
241+
Uses :meth:`_resolve_package` to resolve ``args.target`` to a
242+
specific Chronus package when one is provided (explicit path,
243+
bare name, or ``.`` from the package root). When no target is
244+
given (default glob ``**``), chronus is invoked without a
245+
package argument so it can detect modified packages
246+
interactively.
247+
248+
Optional ``--kind`` and ``--message`` flags are forwarded to
249+
chronus so the developer can skip the interactive prompts
250+
(e.g. ``azpysdk changelog add . --kind breaking -m "Removed foo API"``).
298251
"""
299252
chronus_args = ["add"]
300-
detected_from_cwd = not args.package
301-
package = self._resolve_package(args.package)
302-
if package and detected_from_cwd:
303-
logger.info(f"Detected package from current directory: {package}")
253+
package = self._resolve_package(args)
304254
if package:
305255
chronus_args.append(package)
306256

@@ -319,21 +269,19 @@ def _run_verify(self, args: argparse.Namespace) -> int:
319269
def _run_create(self, args: argparse.Namespace) -> int:
320270
"""Run ``chronus changelog`` to generate CHANGELOG.md files.
321271
322-
When no *package* argument is given but CWD is inside a package
323-
directory, the package path is detected automatically and passed
324-
via ``--package`` so only that package's changelog is generated.
272+
Requires a package to be resolvable from ``args.target`` (via
273+
:meth:`_resolve_package`). Run from the package root with ``.``
274+
or pass the package path/name explicitly.
325275
"""
326276
chronus_args = ["changelog"]
327-
detected_from_cwd = not args.package
328-
package = self._resolve_package(args.package)
329-
if package and detected_from_cwd:
330-
logger.info(f"Detected package from current directory: {package}")
277+
package = self._resolve_package(args)
331278
if not package:
332279
logger.error(
333-
"No package specified and could not detect one from the current directory.\n"
334-
"Either run from within a package directory (e.g. sdk/core/azure-core) or\n"
335-
"pass the package path explicitly:\n\n"
280+
"No package specified or discovered for target. Either run from a package\n"
281+
"root with '.' or pass the package path/name explicitly:\n\n"
336282
" azpysdk changelog create sdk/core/azure-core\n"
283+
" azpysdk changelog create azure-core\n"
284+
" azpysdk changelog create . # from the package root\n"
337285
)
338286
return 1
339287
chronus_args.extend(["--package", package])
@@ -349,15 +297,12 @@ def _run_create(self, args: argparse.Namespace) -> int:
349297
def _run_status(self, args: argparse.Namespace) -> int:
350298
"""Run ``chronus status`` to show pending changes.
351299
352-
When no *package* argument is given but CWD is inside a package
353-
directory, the package path is detected automatically and passed
354-
via ``--only`` so only that package's status is shown.
300+
When ``args.target`` resolves to a specific package, chronus is
301+
scoped to it via ``--only``. Otherwise status is shown for all
302+
packages.
355303
"""
356304
chronus_args = ["status"]
357-
detected_from_cwd = not args.package
358-
package = self._resolve_package(args.package)
359-
if package and detected_from_cwd:
360-
logger.info(f"Detected package from current directory: {package}")
305+
package = self._resolve_package(args)
361306
if package:
362307
chronus_args.extend(["--only", package])
363308
return self._run_chronus(chronus_args)

0 commit comments

Comments
 (0)