refactor: split marketplace commands into package modules#1024
refactor: split marketplace commands into package modules#1024danielmeppiel merged 18 commits intomicrosoft:mainfrom
Conversation
shreejaykurhade
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
|
@danielmeppiel @sergio-sisternes-epam please review |
sergio-sisternes-epam
added
the
panel-review
|
@shreejaykurhade good one - please use pr-description-skill to post large refactoring like this next time |
danielmeppiel
removed
the
panel-review
APM Review Panel Verdict: REJECT
Required before merge (1 item)
Nits (10 items, skip if you want)
CEO arbitrationThe panel is unanimous in substance and nearly so in severity: five of six active panelists reviewed the change, three independently flagged the same defect (stdlib Strategically, this PR is exactly the kind of contribution APM needs to cultivate. An external community member (@shreejaykurhade) voluntarily took on a thankless structural refactor in one of our most strategically important subsystems -- marketplace authoring commands -- and delivered a clean, behavior-preserving split that moves 14 files and nets a slight line reduction. The remaining ~1200 lines in Dissent resolved: cli-logging-expert flagged the subprocess re-export as a NIT; python-architect flagged it as REQUIRED. The CEO sides with python-architect. This is not a cosmetic preference -- it is an API surface defect that will be inherited by every future module extracted from Growth/positioning note: Strong community health signal from @shreejaykurhade tackling structural refactor #821 in marketplace authoring. Three actions: (1) Name the contributor in the next release notes. (2) File a follow-up good-first-issue for extracting consumer commands from Per-persona findings (full)Python ArchitectclassDiagram
direction TB
class MarketplaceGroup {
<<click.Group>>
+_consumer_commands list
+_authoring_commands list
+_authoring_visible() bool
+format_commands(ctx, formatter)
}
class CommandLogger {
<<Base>>
+start(msg)
+progress(msg)
+success(msg)
+error(msg)
+verbose_detail(msg)
}
class _OutdatedRow {
<<ValueObject>>
+name str
+current str
+latest_in_range str
+status str
}
class _CheckResult {
<<ValueObject>>
+name str
+reachable bool
+version_found bool
+error str
}
class _DoctorCheck {
<<ValueObject>>
+name str
+passed bool
+detail str
}
class marketplace_init {
<<Package __init__ 1204 lines>>
+marketplace ClickGroup
+add() consumer cmd
+list_cmd() consumer cmd
+browse() consumer cmd
+update() consumer cmd
+remove() consumer cmd
+search() standalone cmd
+_load_yml_or_exit()
+_render_build_error()
+_render_build_table()
}
class build_mod {
<<ExtractedModule>>
+build() click cmd
}
class check_mod {
<<ExtractedModule>>
+check() click cmd
}
class doctor_mod {
<<ExtractedModule -- REQUIRED>>
+doctor() click cmd
}
class outdated_mod {
<<ExtractedModule>>
+outdated() click cmd
}
class publish_mod {
<<ExtractedModule>>
+publish() click cmd
}
class plugin_pkg {
<<ExtractedSubpackage>>
+package ClickGroup
+add() click cmd
+remove() click cmd
+set_cmd() click cmd
}
MarketplaceGroup <|-- marketplace_init : cls=MarketplaceGroup
marketplace_init *-- build_mod : bottom import
marketplace_init *-- check_mod : bottom import
marketplace_init *-- doctor_mod : bottom import
marketplace_init *-- outdated_mod : bottom import
marketplace_init *-- publish_mod : bottom import
marketplace_init *-- plugin_pkg : bottom import
build_mod ..> marketplace_init : from . import marketplace helpers
check_mod ..> marketplace_init : from . import marketplace helpers
doctor_mod ..> marketplace_init : from . import marketplace subprocess
outdated_mod ..> marketplace_init : from . import marketplace helpers
publish_mod ..> marketplace_init : from . import marketplace helpers
build_mod ..> CommandLogger : uses
check_mod ..> CommandLogger : uses
doctor_mod ..> CommandLogger : uses
outdated_mod ..> CommandLogger : uses
publish_mod ..> CommandLogger : uses
check_mod ..> _CheckResult : creates
outdated_mod ..> _OutdatedRow : creates
doctor_mod ..> _DoctorCheck : creates
flowchart TD
A["apm marketplace build --dry-run"] --> B["cli.py imports marketplace group"]
B --> C["marketplace/__init__.py\nModule load: 46 top-level imports\ndefine MarketplaceGroup, helpers, inline cmds"]
C --> D["__init__.py bottom imports\nfrom .build import build\nfrom .check import check ..."]
D --> E["build.py:12\nfrom . import marketplace, helpers, BuildOptions"]
E --> F["Circular resolved: __init__ already defined\nall symbols before bottom imports execute"]
F --> G["Click dispatch: MarketplaceGroup\nroutes build to build.build()"]
G --> H["build.py:24 _require_authoring_flag()"]
H --> I["build.py:25 CommandLogger marketplace-build"]
I --> J["build.py:29 _load_yml_or_exit logger"]
J --> K["build.py:23 Deferred: from . import MarketplaceBuilder"]
K --> L{"build.py:32 dry_run?"}
L -->|yes| M["logger.info preview only"]
L -->|no| N["MarketplaceBuilder.build\nresolves refs, writes marketplace.json"]
N --> O{"Result?"}
O -->|success| P["_render_build_table logger, report"]
O -->|BuildError| Q["_render_build_error logger, exc\nsys.exit 1"]
O -->|MarketplaceYmlError| R["logger.error str exc\nsys.exit 2"]
style E fill:#fff3b0,stroke:#d47600
style K fill:#fff3b0,stroke:#d47600
Required:
Nits:
CLI Logging ExpertNo findings. (All nits below are pre-existing code moved intact.) Nits:
DevX UX ExpertNo required findings. All user-visible CLI behavior (help text, command names, arguments, options, error messages) is faithfully preserved. Compatibility shim in Nits:
Supply Chain Security ExpertNo required findings. All security-critical code paths (path traversal guards via Nits:
Auth ExpertInactive -- PR #1024 restructures OSS Growth HackerNo required findings. No CHANGELOG entry needed for a pure refactor. Nits:
Verdict computed deterministically: 1 required finding across 5 active panelists. APPROVE iff N == 0. Push a new commit to clear this verdict label automatically.
|
github-actions
Bot
added
panel-rejected
There was a problem hiding this comment.
Pull request overview
Refactors the marketplace CLI command implementation from a single large module into a src/apm_cli/commands/marketplace/ package with per-command modules, while keeping import compatibility via wrappers/re-exports.
Changes:
- Split
apm marketplace {init,build,validate,check,outdated,doctor,publish}into dedicated modules undersrc/apm_cli/commands/marketplace/. - Moved the
apm marketplace package {add,set,remove}subgroup intosrc/apm_cli/commands/marketplace/plugin/, keepingmarketplace_plugin.pyas a compatibility wrapper. - Updated integration test troubleshooting docs to point to the new module locations.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/marketplace/README.md | Updates troubleshooting guidance to reference the new marketplace command module layout. |
| src/apm_cli/commands/marketplace_plugin.py | Compatibility wrapper re-exporting the moved package subgroup. |
| src/apm_cli/commands/marketplace/init.py | Keeps Click group wiring + shared helpers; re-exports split command modules. |
| src/apm_cli/commands/marketplace/build.py | Extracted apm marketplace build into its own module. |
| src/apm_cli/commands/marketplace/check.py | Extracted apm marketplace check into its own module. |
| src/apm_cli/commands/marketplace/doctor.py | Extracted apm marketplace doctor into its own module. |
| src/apm_cli/commands/marketplace/init.py | Extracted apm marketplace init into its own module. |
| src/apm_cli/commands/marketplace/outdated.py | Extracted apm marketplace outdated into its own module. |
| src/apm_cli/commands/marketplace/publish.py | Extracted apm marketplace publish into its own module. |
| src/apm_cli/commands/marketplace/validate.py | Extracted apm marketplace validate into its own module. |
| src/apm_cli/commands/marketplace/plugin/init.py | New package subgroup wiring + shared helper functions for package commands. |
| src/apm_cli/commands/marketplace/plugin/add.py | Extracted apm marketplace package add into its own module. |
| src/apm_cli/commands/marketplace/plugin/set.py | Extracted apm marketplace package set into its own module. |
| src/apm_cli/commands/marketplace/plugin/remove.py | Extracted apm marketplace package remove into its own module. |
| # Version-based — no ref resolution needed. | ||
| if version is not None: | ||
| return None | ||
|
|
||
| # Already a concrete SHA — store as-is. | ||
| if ref is not None and _SHA_RE.match(ref): |
There was a problem hiding this comment.
This file uses Unicode em dash characters (U+2014) in comments (e.g., "Version-based ..." / "Already a concrete SHA ..."). The repo encoding rule requires printable ASCII only for source files; please replace the em dashes with ASCII '-' or '--' to avoid Windows cp1252 encoding issues.
| # Non-HEAD, non-SHA ref — check whether it is a branch name. | ||
| resolver = RefResolver() | ||
| try: | ||
| remote_refs = resolver.list_remote_refs(source) | ||
| except (GitLsRemoteError, OfflineMissError): | ||
| # Cannot verify — store as-is but warn the user. | ||
| logger.warning( |
There was a problem hiding this comment.
Non-ASCII Unicode em dash characters (U+2014) are used in comments here ("Non-HEAD, non-SHA ref ..." and "Cannot verify ..."). Please switch these to ASCII '-' / '--' so the source stays within printable ASCII per the repo encoding rule.
| ) | ||
| return remote_ref.sha | ||
|
|
||
| # Not a branch — tag or unknown ref; store as-is. |
There was a problem hiding this comment.
This comment contains a Unicode em dash (U+2014): "Not a branch ... tag or unknown ref". Please replace it with ASCII punctuation to keep the source file printable-ASCII-only.
| # Not a branch — tag or unknown ref; store as-is. | |
| # Not a branch, tag, or unknown ref; store as-is. |
|
@danielmeppiel done |
danielmeppiel
added
panel-review
# Conflicts: # src/apm_cli/commands/marketplace/__init__.py
…pline
Address review-panel REQUIRED finding plus architect feedback:
REQUIRED FIX (search alias regression):
- Remove 'search' from MarketplaceGroup._consumer_commands and drop the
marketplace.add_command(search) call. 'apm search' remains the canonical
top-level command (registered in cli.py); the new 'apm marketplace search'
alias was an unintentional surface added by the refactor split.
- Update tests/unit/commands/test_marketplace_gating.py to reflect that
'search' is not part of the marketplace group's consumer command set.
IMPORT DISCIPLINE (architect feedback):
- Submodules (build, check, doctor, init, outdated, publish, validate,
plugin/{add,remove,set}) now import domain types from their canonical
source modules (...core.command_logger, ...marketplace.builder,
...marketplace.errors, etc.) instead of re-importing them via the
package __init__. Drop redundant lazy 'from . import X' calls inside
command bodies.
- Package __init__ keeps eager re-exports (used by test mock.patch and
by the helpers that still live in __init__) but the submodule code
paths no longer rely on them.
- Remove dead 'import subprocess' from package __init__.
CLEANUP:
- Delete src/apm_cli/commands/marketplace_plugin.py compatibility shim.
Its sole consumer (tests/unit/commands/test_marketplace_plugin.py)
now imports from apm_cli.commands.marketplace.plugin directly.
- Update mock.patch paths in unit + integration tests to point at the
canonical submodule namespace (e.g. marketplace.build.MarketplaceBuilder
instead of marketplace.MarketplaceBuilder).
Verified: 6702 unit tests pass; 'apm marketplace --help' lists the
correct consumer set (no 'search'); 'from apm_cli.commands.marketplace
import marketplace, search' still works.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update: review-panel feedback addressed (commit 7299f55)Fixed the REQUIRED finding from the local review panel and the import-discipline issues flagged by python-architect. REQUIRED fix: search alias regression
Import discipline (architect feedback)
Cleanup
Verification
Deliberately punted to follow-up (architect's call, pragmatic for this PR)
These are tracked as low-priority follow-ups; not blocking for this refactor's goal. |
danielmeppiel
added
panel-review
…/marketplace # Conflicts: # src/apm_cli/commands/marketplace/__init__.py # src/apm_cli/commands/marketplace_plugin.py # tests/unit/commands/test_marketplace_check.py
|
@danielmeppiel please check as ci cd are failing and merge are still happening. it's not feasible for me to maintain this refactor as most changes are occuring in these files. Would love if you approve this earlier. |
# Conflicts: # src/apm_cli/commands/marketplace/__init__.py # src/apm_cli/commands/marketplace_plugin.py # tests/unit/commands/test_marketplace_build.py # tests/unit/commands/test_marketplace_gating.py
|
@danielmeppiel please review |
danielmeppiel
added
panel-review
We had some problems with the agentic PR review, it should come in now. We do best effort on reviewing and want to get this one in. Thank you! |
APM Review Panel Verdict: REJECT
Required before merge (6 items)
Nits (11 items, skip if you want)
CEO arbitrationThe panel's most urgent finding is a fact-regression, not an opinion: three independent panelists (cli-logging-expert, devx-ux-expert, oss-growth-hacker) independently identified that the gitignore warning now names "marketplace.yml" instead of "apm.yml". This is not a style preference -- it is an actively wrong recovery instruction that will send users hunting for a file that no longer exists. The triple-flagging constitutes the strongest possible panel consensus short of unanimity, and the fix is a one-line string restore. This alone is sufficient to block the PR as-submitted. On scope: the python-architect's finding that The devx-ux-expert's finding on Dissent resolved: The only material dissent is on severity of Growth/positioning note: The Per-persona findings (full)Python ArchitectclassDiagram
direction TB
class MarketplaceInit {
<<Package __init__ 1277 LOC>>
+MarketplaceGroup
+marketplace : click.Group
+_load_config_or_exit()
+_check_gitignore_for_marketplace_json()
+_require_authoring_flag()
+_render_check_table()
+_render_doctor_table()
+_render_publish_plan()
+_render_publish_summary()
+add() +list_cmd() +browse()
+update() +remove() +search()
}
class CheckModule {
<<SubModule check.py>>
+check()
}
class DoctorModule {
<<SubModule doctor.py>>
+doctor()
}
class InitModule {
<<SubModule init.py>>
+init()
}
class MigrateModule {
<<SubModule migrate.py>>
+migrate()
}
class OutdatedModule {
<<SubModule outdated.py>>
+outdated()
}
class PublishModule {
<<SubModule publish.py>>
+publish()
}
class ValidateModule {
<<SubModule validate.py>>
+validate()
}
class PluginPackage {
<<SubPackage plugin>>
+package : click.Group
+add() +set() +remove()
}
class CommandLogger {
<<Base>>
+start() +progress() +success()
+warning() +error()
+blank_line()
}
class MarketplaceInit:::touched
class CheckModule:::touched
class DoctorModule:::touched
class InitModule:::touched
class MigrateModule:::touched
class OutdatedModule:::touched
class PublishModule:::touched
class ValidateModule:::touched
class PluginPackage:::touched
class CommandLogger:::touched
MarketplaceInit *-- MarketplaceGroup : defines
CheckModule ..> MarketplaceInit : imports marketplace + helpers
DoctorModule ..> MarketplaceInit : imports marketplace + helpers
InitModule ..> MarketplaceInit : imports marketplace + helpers
MigrateModule ..> MarketplaceInit : imports marketplace + helpers
OutdatedModule ..> MarketplaceInit : imports marketplace + helpers
PublishModule ..> MarketplaceInit : imports marketplace + helpers
ValidateModule ..> MarketplaceInit : imports marketplace + helpers
PluginPackage ..> MarketplaceInit : imports marketplace
MarketplaceInit ..> PluginPackage : late-imports package
CheckModule ..> CommandLogger : instantiates
DoctorModule ..> CommandLogger : instantiates
note for MarketplaceInit "REQUIRED: still 1277 LOC. Consumer commands and render helpers not yet extracted."
note for CheckModule "_require_authoring_flag() imported but function is a no-op stub"
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A(["$ apm marketplace check"]) --> B["click dispatches to check.py::check()"]
B --> C["_require_authoring_flag()\nmarketplace/__init__.py:200\nDEAD NOP - returns None"]
C --> D["CommandLogger('marketplace-check')\ncore/command_logger.py"]
D --> E["_load_config_or_exit(logger)\nmarketplace/__init__.py:144\nIO reads apm.yml or marketplace.yml"]
E -->|MarketplaceYmlError| F["logger.error() + sys.exit(1 or 2)"]
E -->|ok| G["_warn_duplicate_names(logger, yml)\nmarketplace/__init__.py:169"]
G --> H{"--offline flag?"}
H -->|yes| I["schema-only check path\nIO read marketplace.yml"]
H -->|no| J["RefResolver.list_remote_refs()\nNET git ls-remote per entry"]
J -->|GitLsRemoteError| K["_render_check_table with error result\nmarketplace/__init__.py:804"]
J -->|ok| L["_extract_tag_versions()\nmarketplace/__init__.py:724\nsatisfies_range() semver check"]
L --> K
I --> K
K --> M["sys.exit(0) or sys.exit(1)"]
Design patterns
Required findings:
Nits:
CLI Logging ExpertRequired:
Nits:
DevX UX ExpertRequired:
Nits:
Supply Chain Security ExpertNo findings. Nits:
Auth ExpertInactive -- No auth files touched; PR refactors marketplace.py into a package with only import-path depth changes (..core.auth -> ...core.auth), no token or credential logic changed. OSS Growth HackerRequired:
Nits:
Verdict computed deterministically: 6 required findings across 5 active panelists. APPROVE iff N == 0. Push a new commit to clear this verdict label automatically. Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
github-actions
Bot
added
panel-rejected
…help text and warning - Delete _require_authoring_flag() no-op and remove all 7 import+call sites (check, doctor, init, migrate, outdated, publish, plugin/__init__). - Restore 'apm marketplace init' help text scaffold affordance: '(scaffolds apm.yml if missing)'. - Fix gitignore warning to reference apm.yml instead of marketplace.yml, matching the post-migration canonical config. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
All the test are passing. let's see how this goes. |
Description
Refactors the marketplace CLI implementation by replacing the monolithic
src/apm_cli/commands/marketplace.pyfile with a dedicatedsrc/apm_cli/commands/marketplace/package using per-command modules.This improves maintainability and reviewability while preserving existing CLI behavior, import compatibility, and marketplace authoring guardrails.
Fixes #821
Type of change
Testing
Tested with:
python -m compileall -q src/apm_cli/commands/marketplace src/apm_cli/commands/marketplace_plugin.py
python -m pytest tests/unit/commands/test_marketplace_build.py tests/unit/commands/test_marketplace_check.py tests/unit/commands/test_marketplace_doctor.py tests/unit/commands/test_marketplace_gating.py tests/unit/commands/test_marketplace_init.py tests/unit/commands/test_marketplace_outdated.py tests/unit/commands/test_marketplace_plugin.py tests/unit/commands/test_marketplace_publish.py tests/unit/marketplace/test_marketplace_commands.py tests/unit/marketplace/test_marketplace_validator.py -q