Skip to content

Add pr-best-practices skill#504

Open
Thanhphan1147 wants to merge 7 commits intomainfrom
add_pr_best_practices_skill
Open

Add pr-best-practices skill#504
Thanhphan1147 wants to merge 7 commits intomainfrom
add_pr_best_practices_skill

Conversation

@Thanhphan1147
Copy link
Copy Markdown
Collaborator

@Thanhphan1147 Thanhphan1147 commented May 4, 2026 •
edited
Loading

What this PR does

Add a skill that when called will analyze the changes between the first and final iteration of a PR authored by an LLM. The result is a set of good and bad example maintained in .github/instructions/best-practices.instructions.md. For example:

Do

  • Gate service-dependent refresh logic on service readiness before calling the policy API from _reconcile — relation data can exist before the snap service is ready to answer requests (PR #475)

Before review — refresh happened unconditionally during reconcile

self._fetch_and_refresh_backend_requests(
    haproxy_route_policy_information, haproxy_route_policy_requirer_data
)
if (proxied_endpoint := haproxy_route_policy_requirer_data.proxied_endpoint) and (
    host := proxied_endpoint.host
):
    allowed_hosts.append(host)

After review — only refresh when the service is actually running

if is_service_active():
    # We can only send requests to the policy API if the service is active.
    self._fetch_and_refresh_backend_requests(
        haproxy_route_policy_information, haproxy_route_policy_requirer_data
    )

if proxied_endpoint := haproxy_route_policy_requirer_data.proxied_endpoint:
    allowed_hosts.append(proxied_endpoint)

Checklist

  • I followed the contributing guide
  • I added or updated the documentation (if applicable)
  • I updated docs/changelog.md with user-relevant changes
  • I added a change artifact for user-relevant changes in docs/release-notes/artifacts. If no change artifact is necessary, I tagged the PR with the label no-release-note.
  • I used AI to assist with preparing this PR
  • I added or updated tests as needed (unit and integration)
  • If integration test modules are used: I updated the workflow configuration
    (e.g., in .github/workflows/integration_tests.yaml, ensure the modules list is correct)
  • If this PR involves a Grafana dashboard: I added a screenshot of the dashboard
  • If this PR involves Terraform: terraform fmt passes and tflint reports no errors

@Thanhphan1147 Thanhphan1147 requested a review from a team as a code owner May 4, 2026 15:11
Thanhphan1147 and others added 3 commits May 4, 2026 17:12
@Thanhphan1147
add pr-best-practices skill and initial best-practices instructions
a4d788b 
- .github/skills/pr-best-practices/SKILL.md: skill that analyzes merged
  LLM-authored PRs, compares first vs final commit, and synthesizes do/don't
  rules with concrete code examples into best-practices.instructions.md
- .github/skills/pr-best-practices/evals/evals.json: 3 eval cases covering
  substantive PR, validation-fix PR, and trivial (non-significant) PR
- .github/instructions/best-practices.instructions.md: initial rules extracted
  from PR #475 (relation contracts, reconcile flow, testing) and PR #500
  (validation error handling) with before/after code examples
- .gitignore: ignore *-workspace/ directories (skill eval scratch data)

Co-authored-by: Copilot <[email protected]>
@Thanhphan1147
chore(skill): update pr-best-practices with repo-specific file hints
fd7aa4d 
- Clarify description: 'after human reviews and code changes' (was 'after human review')
- Replace generic file extension list with explicit repo-aware globs:
  **/*.py (charm/test code), haproxy-operator/*.j2 (HAProxy templates),
  **/*.yaml (charmcraft/snapcraft manifests)
- Drop .github/workflows/*.yaml — CI config doesn't yield coding best practices
- Fix typo **/*yaml → **/*.yaml; strip trailing whitespace

Co-authored-by: Copilot <[email protected]>
@Thanhphan1147
remove file
f0f5fe1
@Thanhphan1147 Thanhphan1147 changed the title remove file Add pr-best-practices skil May 4, 2026
@Thanhphan1147 Thanhphan1147 changed the title Add pr-best-practices skil Add pr-best-practices skill May 4, 2026
@Thanhphan1147 Thanhphan1147 added the no-release-note This PR does not require a change artifact label May 4, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

Test results for commit 4289e39

Test coverage for 4289e39

Name                                       Stmts   Miss Branch BrPart  Cover   Missing
--------------------------------------------------------------------------------------
lib/charms/haproxy/v0/ddos_protection.py     154     51     34      8    64%   157-174, 183-187, 288, 316-318, 323, 326-330, 342-344, 381, 387, 393, 396, 424, 495-498, 510-529
src/charm.py                                  21      0      0      0   100%
src/state.py                                  43      0      0      0   100%
--------------------------------------------------------------------------------------
TOTAL                                        218     51     34      8    73%

Static code analysis report

Run started:2026-05-07 09:30:25.490846+00:00

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 333
  Total lines skipped (#nosec): 0
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

Test results for commit 4289e39

Test coverage for 4289e39

Name                                         Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------------------------------------
lib/charms/haproxy/v0/ddos_protection.py       154     42     34      3    72%   157-174, 183-187, 265, 284, 415-418, 422-424, 459-478, 514-529
lib/charms/haproxy/v0/spoe_auth.py             158     55     32      2    59%   203, 304-306, 315, 354-381, 392-402, 441-442, 459-472, 484-501, 522-525, 529-531
lib/charms/haproxy/v1/haproxy_route_tcp.py     385    153     78      8    56%   209, 212, 281, 290-293, 297-300, 318-321, 336, 342-347, 447, 452, 829-832, 836, 863-874, 897-900, 904-906, 926-928, 1042-1083, 1087-1093, 1097, 1166-1195, 1266-1305, 1335-1337, 1362-1364, 1386-1390, 1409-1411, 1429-1431, 1438-1444, 1452-1454, 1462-1463, 1474-1481, 1494-1505, 1513-1534, 1546-1547, 1558-1559, 1570-1573, 1584-1585, 1614-1623, 1639-1642, 1658-1669, 1685-1688, 1706-1717, 1728-1729, 1737-1738, 1746-1747, 1758-1761
lib/charms/haproxy/v2/haproxy_route.py         386     54     98     27    82%   181, 202, 257, 266-269, 294-297, 318-323, 673-674, 867->exit, 874, 900-911, 934-937, 941-943, 962-964, 1136-1142, 1146, 1343->1345, 1347->1349, 1349->1351, 1351->1353, 1353->1355, 1355->1358, 1393, 1401, 1406, 1409, 1434, 1462, 1466, 1470, 1493, 1513, 1522-1523, 1525->exit, 1561-1563, 1583, 1597, 1602-1604
src/charm.py                                   293     71     84     13    71%   102, 228, 236-252, 257, 262, 280, 291, 297-298, 332-352, 371, 471, 478-486, 514-527, 540-545, 554, 567-568, 575, 585, 595, 601-607, 623, 674-677, 683->682, 696-699
src/haproxy.py                                 125     31      6      2    75%   108-114, 134-156, 266-267, 270, 278-284, 312, 342-353, 365-367, 377-378
src/http_interface.py                           73     25      4      0    62%   74, 83, 92, 106-108, 126, 138, 150, 162, 170-175, 187, 194, 202, 217-227
src/state/charm_state.py                        78     15     14      4    79%   94-96, 101-102, 105, 150-155, 164, 216-218, 230-231
src/state/ddos_protection.py                    39      0      2      0   100%
src/state/exception.py                           1      0      0      0   100%
src/state/ha.py                                 30      1      2      1    94%   50
src/state/haproxy_route.py                     283     46     76      8    82%   161, 190-199, 256, 281, 332-360, 369, 378, 387, 396, 408, 446-472, 528, 568, 584-585, 602, 822-835
src/state/haproxy_route_tcp.py                 120     17     42      1    80%   92-94, 109->112, 147-160
src/state/ingress.py                            38      0      4      0   100%
src/state/ingress_per_unit.py                   32      0      4      0   100%
src/state/spoe_auth.py                          26      2      2      0    93%   63-64
src/state/tls.py                                39      7     12      4    78%   74, 77-78, 127-135, 141-142
src/state/validation.py                         46     23      8      1    44%   66-67, 71-98
src/tls_relation.py                             62      3     14      3    92%   87->86, 119-129, 141->143
----------------------------------------------------------------------------------------
TOTAL                                         2368    545    516     77    74%

Static code analysis report

Working... ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00
Run started:2026-05-07 09:30:48.441604+00:00

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 10469
  Total lines skipped (#nosec): 13
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 9

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

Test results for commit 4289e39

Test coverage for 4289e39

Name                      Stmts   Miss Branch BrPart  Cover   Missing
---------------------------------------------------------------------
src/charm.py                 88     29     14      3    63%   97-98, 108-113, 138, 145-157, 161-176, 184-199
src/policy.py                66     37      2      0    43%   30-32, 37-38, 43-53, 58-59, 64-65, 78-92, 115-117, 140-141, 159-176, 187-197, 208-211
src/state/database.py        32      2      6      2    89%   76, 79
src/state/policy.py          62      2     10      2    94%   94, 106
src/state/validation.py      44     18      0      0    59%   57-59, 61-63, 74-87
---------------------------------------------------------------------
TOTAL                       292     88     32      7    68%

Static code analysis report

Run started:2026-05-07 09:30:24.380780+00:00

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 1152
  Total lines skipped (#nosec): 10
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

Test results for commit 4289e39

Test coverage for 4289e39

Name                                        Stmts   Miss  Cover
---------------------------------------------------------------
haproxy_route_policy/__init__.py                0      0   100%
haproxy_route_policy/settings.py               30      1    97%
haproxy_route_policy/test_settings.py           4      0   100%
haproxy_route_policy/urls.py                    5      0   100%
manage.py                                      11      2    82%
policy/__init__.py                              0      0   100%
policy/admin.py                                22      5    77%
policy/apps.py                                  3      0   100%
policy/db_models.py                            50      2    96%
policy/management/__init__.py                   0      0   100%
policy/management/commands/__init__.py          0      0   100%
policy/middleware.py                           16      4    75%
policy/migrations/0001_initial.py               7      0   100%
policy/migrations/0002_rule.py                  5      0   100%
policy/migrations/0003_alter_rule_kind.py       4      0   100%
policy/migrations/__init__.py                   0      0   100%
policy/rule_engine.py                          42      6    86%
policy/serializers.py                          30      4    87%
policy/tests/__init__.py                        0      0   100%
policy/tests/test_auth.py                      36     20    44%
policy/tests/test_models.py                    85      0   100%
policy/tests/test_rule_engine.py              117      0   100%
policy/tests/test_views.py                    201      0   100%
policy/urls.py                                  3      0   100%
policy/views.py                                95     16    83%
---------------------------------------------------------------
TOTAL                                         766     60    92%

Static code analysis report

Run started:2026-05-07 09:30:29.838309+00:00

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 1680
  Total lines skipped (#nosec): 1
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

Test results for commit 4289e39

Test coverage for 4289e39

Name                               Stmts   Miss Branch BrPart  Cover   Missing
------------------------------------------------------------------------------
src/charm.py                          45      9      2      0    77%   65-91, 96-98
src/haproxy_spoe_auth_service.py      44     16      2      0    61%   56-64, 76-82, 93-117
src/state.py                          55     15      6      1    67%   64-66, 79, 125-146
------------------------------------------------------------------------------
TOTAL                                144     40     10      1    68%

Static code analysis report

Run started:2026-05-07 09:30:25.302518

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 409
  Total lines skipped (#nosec): 1
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 1

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alithethird alithethird alithethird approved these changes

Assignees

No one assigned

Labels

Libraries: Out of sync no-release-note This PR does not require a change artifact

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Thanhphan1147 @alithethird