diff options
| author | Yuriy Andamasov <yuriy@vyos.io> | 2026-04-17 15:22:01 +0300 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2026-04-17 13:22:01 +0100 |
| commit | db645c3fc5a66f0fdadd03b5096937f70787e3b0 (patch) | |
| tree | cc4b894829c718a5e906137e03b2513a2ba788e0 | |
| parent | 1bee3f7d8e1dca104c0b5c5d7a7833946aa7514a (diff) | |
| download | vyos.vyos-db645c3fc5a66f0fdadd03b5096937f70787e3b0.tar.gz vyos.vyos-db645c3fc5a66f0fdadd03b5096937f70787e3b0.zip | |
T8523: add Copilot custom review instructions (#465)
* Add easy-wins improvement spec for vyos.vyos collection
Covers five phases: formatting compliance, runtime.yml redirect fix,
deprecated feature cleanup, missing unit tests, and template deduplication.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Add implementation plan for easy-wins improvements
15 tasks across 5 phases: formatting compliance, runtime.yml bugfix,
deprecated code cleanup, missing unit tests, template deduplication.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Update spec and plan with architect review corrections
Key changes:
- Defer Phase 5 (template deduplication) to v7.0.0: route_maps
are not identical, BGP dedup blocked by Python module-level
scoping, OSPF has fundamentally different command paradigms
- Add .git-blame-ignore-revs step to Phase 1
- Add missing test cases: overridden/rendered for resource modules,
aggregate/purge/with_address for vyos_vlan
- Fix incorrect claim that version.py LooseVersion is unused
- Add sequential merge requirement to preamble
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* chore: add .worktrees/ and .claude/ to .gitignore
* T8523: add Copilot custom review instructions
Add .github/copilot-instructions.md and path-specific instruction
files under .github/instructions/ to teach Copilot project conventions:
copyright attribution, changelog fragment key selection, test mocking
patterns, required state coverage for resource modules, and module
option description standards.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* T8523: remove copyright instructions from Copilot config
Copyright attribution rules are internal guidance kept in Claude
config, not in Copilot review instructions.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* T8523: remove docs/superpowers files inadvertently included
These spec/plan files came from local main commits that were
pulled in during rebase. They are unrelated to this PR.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* T8523: fix inaccuracies flagged by Copilot review
- CLI quoting: clarify that boolean flags and bare keywords are
unquoted; only string/address values use single quotes
- Test mocking: describe get_device_data / execute_show_command as
the primary resource module pattern (used by most existing tests);
note get_resource_connection_facts approach as an alternative
- modules.instructions.md: broaden applyTo to include meta/runtime.yml
so redirect guidance fires when that file is edited
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* T8523: fix legacy module mocking guidance in tests.instructions.md
Existing legacy tests use side_effect for run_commands, not
return_value. Updated to describe side_effect as the standard
approach, note return_value as also acceptable for fixed responses,
and clarify that the patched helper varies by module (run_commands
for command/facts/ping, load_config/get_config for vyos_config).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* T8523: fix four more inaccuracies flagged by Copilot review
- CLI quoting: address values in firewall groups are unquoted in
fixtures; instruct reviewers to align with surrounding fixture
style rather than flagging all unquoted addresses
- Resource module mocking: add .start()/.stop() calls to the snippet
so it matches the actual setUp/tearDown pattern in existing tests
- terminal plugin versions: reference README.md instead of hardcoding
a version list that diverges from README
- changelog fragment: remove stale "copyright headers" claim (that
section was dropped from the instruction files)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| -rw-r--r-- | .github/copilot-instructions.md | 49 | ||||
| -rw-r--r-- | .github/instructions/modules.instructions.md | 37 | ||||
| -rw-r--r-- | .github/instructions/tests.instructions.md | 114 | ||||
| -rw-r--r-- | .gitignore | 6 | ||||
| -rw-r--r-- | changelogs/fragments/copilot-instructions.yml | 4 |
5 files changed, 210 insertions, 0 deletions
diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 00000000..fc25faa1 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,49 @@ +# Copilot Review Instructions — vyos.vyos + +This is the `vyos.vyos` Ansible network collection for managing VyOS devices. +Namespace `vyos`, name `vyos`, version `6.0.0`. All modules are prefixed `vyos_`. + +## Commit and PR standards + +- Every commit title must start with a Phorge task ID: `T<number>: description`. +- Every PR must have exactly one changelog fragment in `changelogs/fragments/`. +- PR descriptions that state a test count (e.g. "Add 8 unit tests") must match the actual number of test methods in the changed files. Flag mismatches. + +## Changelog fragments + +Fragments are YAML files under `changelogs/fragments/`. Valid top-level keys: + +| Key | Use for | +|-----|---------| +| `trivial` | Developer tooling, CI, housekeeping, formatting-only changes | +| `bugfixes` | Bug fixes | +| `minor_changes` | New features or user-visible improvements | +| `major_changes` | Breaking changes | +| `security_fixes` | Security fixes | +| `doc_changes` | Documentation-only changes | + +Flag any fragment that uses `minor_changes` for what is actually developer tooling (linting, formatting, gitignore, test scaffolding). Those should use `trivial`. + +## Module architecture + +Two module families: + +**Resource modules** (`vyos_interfaces`, `vyos_firewall_rules`, `vyos_bgp_global`, etc.) follow a four-part structure under `plugins/module_utils/network/vyos/`: +- `argspec/{resource}/` — argument spec +- `config/{resource}/` — config builder +- `facts/{resource}/` — facts parser +- `rm_templates/{resource}.py` — regex/Jinja2 CLI templates + +Resource modules support all states: `merged`, `replaced`, `overridden`, `deleted`, `rendered`, `gathered`, `parsed`. + +**Legacy modules** (`vyos_vlan`, `vyos_config`, `vyos_command`, `vyos_user`, etc.) do not follow the resource module pattern. + +## VyOS CLI conventions + +- Set commands: `set interfaces ethernet eth0 address '192.0.2.1/24'` +- Delete commands: `delete interfaces ethernet eth0 address '192.0.2.1/24'` +- Quoting varies by context. In general, string values (descriptions, names, ELIN numbers) are single-quoted; boolean flags and bare keywords are not. However, address/prefix values may be quoted or unquoted depending on where they appear: + - Quoted: `address '192.0.2.1/24'`, `description 'my-iface'`, `elin '0000000911'` + - Unquoted: `address 192.0.2.1` (in firewall groups), `disable`, `mtu-ignore`, `vif 200` +- When reviewing tests and fixtures, align with the quoting style used by surrounding fixtures rather than flagging a missing quote as an error. +- Interface types: `ethernet`, `loopback`, `bonding`, `bridge`, `tunnel`, `wireguard`. diff --git a/.github/instructions/modules.instructions.md b/.github/instructions/modules.instructions.md new file mode 100644 index 00000000..d7782e1e --- /dev/null +++ b/.github/instructions/modules.instructions.md @@ -0,0 +1,37 @@ +--- +applyTo: "plugins/**,meta/runtime.yml" +--- + +# Plugin conventions + +## Module option descriptions + +- Must be complete English sentences ending with a period. +- No grammar errors. Common mistake: "the number hops" should be "the number of hops". +- Deprecated options must include `removed_in_version` and `removed_from_collection` fields. +- Do not add new options with `deprecated: true` — remove deprecated options entirely. + +## rm_templates files + +Files in `plugins/module_utils/network/vyos/rm_templates/` define regex parsers and Jinja2 generators for a resource module. Each entry has: +- `name` — unique identifier +- `getval` — compiled regex with named groups +- `setval` — Jinja2 template or callable producing a VyOS CLI command +- `result` — dict mapping regex groups to facts structure +- `shared` (optional) — bool, whether the template applies to a shared config block + +## meta/runtime.yml redirects + +Short-name redirects must point to the actual module name with the `vyos_` prefix. For example: +```yaml +snmp_server: + redirect: vyos.vyos.vyos_snmp_server # correct + # NOT: vyos.vyos.vyos_snmp_servers # wrong (pluralized) +``` + +Verify any changed redirect target exists as a real module file under `plugins/modules/`. + +## cliconf / terminal plugins + +`plugins/cliconf/vyos.py` — do not modify without understanding edit-mode and commit semantics. +`plugins/terminal/vyos.py` — handles prompt detection; regex changes require testing against all supported VyOS versions listed in README.md. diff --git a/.github/instructions/tests.instructions.md b/.github/instructions/tests.instructions.md new file mode 100644 index 00000000..150db41e --- /dev/null +++ b/.github/instructions/tests.instructions.md @@ -0,0 +1,114 @@ +--- +applyTo: "tests/unit/**" +--- + +# Unit test conventions + +## Base class and structure + +All test classes inherit from `TestVyosModule` in `tests/unit/modules/network/vyos/vyos_module.py`. + +```python +class TestVyosFooModule(TestVyosModule): + module = vyos_foo + + def setUp(self): ... + def tearDown(self): ... + def load_fixtures(self, commands=None, filename=None): ... + def test_...(self): ... +``` + +## Mocking: resource modules + +Resource module tests require two framework-level patches in `setUp` with corresponding cleanup in `tearDown`: + +```python +# in setUp: +self.mock_get_resource_connection_config = patch( + "ansible_collections.ansible.netcommon.plugins.module_utils.network.common.cfg.base.get_resource_connection" +) +self.get_resource_connection_config = self.mock_get_resource_connection_config.start() + +self.mock_get_resource_connection_facts = patch( + "ansible_collections.ansible.netcommon.plugins.module_utils.network.common.facts.facts.get_resource_connection" +) +self.get_resource_connection_facts = self.mock_get_resource_connection_facts.start() + +# in tearDown: +self.mock_get_resource_connection_config.stop() +self.mock_get_resource_connection_facts.stop() +``` + +Most resource module tests also patch the facts class's `get_device_data` method directly and use it in `load_fixtures`: + +```python +# in setUp: +self.mock_execute_show_command = patch( + "ansible_collections.vyos.vyos.plugins.module_utils.network.vyos." + "facts.{resource}.{resource}.{Resource}Facts.get_device_data" +) +self.execute_show_command = self.mock_execute_show_command.start() + +# in load_fixtures: +def load_from_file(*args, **kwargs): + return load_fixture("vyos_{resource}_config.cfg") +self.execute_show_command.side_effect = load_from_file +``` + +An alternative pattern sets `self.get_resource_connection_facts.return_value.get_config.return_value = fixture_data` in `load_fixtures` instead — both approaches work, but the `get_device_data` pattern is used by most existing tests. + +## Mocking: legacy modules + +Legacy modules (`vyos_vlan`, `vyos_config`, `vyos_command`, etc.) patch the specific helper the module calls, directly in the module under test. Which helper depends on the module: + +```python +# vyos_command, vyos_facts, vyos_ping — patch run_commands +self.mock_run_commands = patch("ansible_collections.vyos.vyos.plugins.modules.vyos_foo.run_commands") +# vyos_config — patch get_config / load_config +self.mock_load_config = patch("ansible_collections.vyos.vyos.plugins.modules.vyos_foo.load_config") +``` + +In `load_fixtures`, configure the mock using `side_effect` (for dynamic fixture loading) or `return_value` (for a fixed response): + +```python +# side_effect — used by most existing legacy tests +def load_from_file(*args, **kwargs): + return load_fixture("vyos_foo_config.cfg") +self.run_commands.side_effect = load_from_file + +# return_value — acceptable for simple fixed responses +self.run_commands.return_value = [SHOW_OUTPUT] +``` + +## load_fixtures and filename + +`execute_module()` always calls `load_fixtures()`. Never set mock return values inside a test method — they will be overwritten by the next `execute_module` call. Use the `filename` parameter to vary the fixture: + +```python +def load_fixtures(self, commands=None, filename=None): + if filename == "empty": + self.run_commands.return_value = [EMPTY_OUTPUT] + else: + self.run_commands.return_value = [DEFAULT_OUTPUT] +``` + +Then call: `self.execute_module(changed=True, commands=commands, filename="empty")` + +## Fixture files + +Raw device CLI output lives in `tests/unit/modules/network/vyos/fixtures/` as `.cfg` files. +Load with `load_fixture("vyos_foo_config.cfg")`. + +## Required test coverage for resource modules + +A complete resource module test file should cover all applicable states: +- `merged` (including an idempotent case) +- `replaced` +- `overridden` +- `deleted` +- `rendered` — assert `result["rendered"]` matches expected CLI commands +- `gathered` — assert `result["gathered"]` contains expected structured data +- `parsed` — pass `running_config=raw_string` and assert `result["parsed"]` + +Flag test files that are missing `rendered`, `gathered`, or `parsed` tests without explanation. + @@ -120,3 +120,9 @@ changelogs/.plugin-cache.yaml inventory.network *.bak + +# Git worktrees +.worktrees/ + +# Claude Code +.claude/ diff --git a/changelogs/fragments/copilot-instructions.yml b/changelogs/fragments/copilot-instructions.yml new file mode 100644 index 00000000..c7af5c74 --- /dev/null +++ b/changelogs/fragments/copilot-instructions.yml @@ -0,0 +1,4 @@ +--- +trivial: + - Add Copilot custom review instructions to guide automated code review with + project-specific conventions for tests, changelog fragments, and module patterns. |
