Skip to content

fix: sonar issues: Get SCM.py compliant with Sonar#1613

Open
jcraiglo1 wants to merge 5 commits into
mainfrom
sonar/scm-regex
Open

fix: sonar issues: Get SCM.py compliant with Sonar#1613
jcraiglo1 wants to merge 5 commits into
mainfrom
sonar/scm-regex

Conversation

@jcraiglo1

@jcraiglo1 jcraiglo1 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Sonarcloud is complaining about a regex in a different PR. This PR gets rid of that regex in favor of utilizing the ansible runner API. Sonar has since scanned this file and determined that the tests only cover 83% of code in this file and is gating. So this PR will also add test cases to the rest of the file.

Assisted-By: Claude

Summary by CodeRabbit

Summary by CodeRabbit

  • Bug Fixes

    • Improved SCM/Ansible execution parsing by using structured failure events to generate more accurate, credential-specific error messages.
    • Enhanced SCM version hash extraction on successful runs, including better handling when version details are missing.
    • Improved clone behavior around SSH/GPG credentials and key management, including proper environment setup.
  • Tests

    • Added/expanded unit tests for proxy environment restoration, SSH key cloning behavior, GPG verification environment, directory creation, decryption/add-GPG success and failure paths, and executor event extraction.

@jcraiglo1
jcraiglo1 requested a review from a team as a code owner July 13, 2026 20:39
@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The SCM executor now reads Ansible runner events to extract git versions and failure messages, while repository tests cover proxy restoration, credential setup, key handling, destination creation, and executor outcomes.

Changes

SCM event handling and repository setup

Layer / File(s) Summary
Event-based execution and error mapping
src/aap_eda/services/project/scm.py, tests/unit/test_scm.py
Replaces redirected-output parsing with Ansible event extraction for SCM hashes and failure messages, preserves SCM exception mappings, updates the unused revision parameter, and tests success and failure outcomes.
Repository credentials and environment
tests/unit/test_scm.py
Covers proxy restoration, SSH credentials, destination creation, GPG verification setup, key decryption, GPG key import, and subprocess failure paths.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant GitAnsibleRunnerExecutor
  participant ansible_runner
  participant EventRecords
  participant SCMExceptions
  GitAnsibleRunnerExecutor->>ansible_runner: run SCM operation
  ansible_runner-->>EventRecords: return runner_on_ok or runner_on_failed events
  GitAnsibleRunnerExecutor->>EventRecords: extract scm_version or failure msg
  GitAnsibleRunnerExecutor->>SCMExceptions: raise mapped SCM error when needed
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title matches the main change: making scm.py Sonar-compliant by replacing regex-based SCM handling.
Description check ✅ Passed The description covers the main change, motivation, and testing intent, but it lacks explicit template sections and details like risks or issue links.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sonar/scm-regex

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov-commenter

codecov-commenter commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.17%. Comparing base (51086aa) to head (cd4276a).

@@            Coverage Diff             @@
##             main    #1613      +/-   ##
==========================================
+ Coverage   92.75%   93.17%   +0.41%     
==========================================
  Files         244      244              
  Lines       11346    11362      +16     
==========================================
+ Hits        10524    10586      +62     
+ Misses        822      776      -46     
Flag Coverage Δ
unit-int-tests-3.11 93.17% <100.00%> (+0.41%) ⬆️
unit-int-tests-3.12 93.17% <100.00%> (+0.41%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/aap_eda/services/project/scm.py 99.13% <100.00%> (+21.56%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jcraiglo1 jcraiglo1 changed the title fix: sonar issues: Extract problematic regex to helper method fix: sonar issues: Get SCM.py compliant with Sonar Jul 14, 2026
@B-Whitt

B-Whitt commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

/run-e2e

@B-Whitt

B-Whitt commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

/run-atf-tests

@aap-pde-ci-bot

Copy link
Copy Markdown

✅ Test Results - PASSED

Summary

Metric Count
Total Tests 65
✅ Passed 49
❌ Failed 0
⚠️ Errors 0
⏭️ Skipped 16
⏱️ Duration 227.41s

Pass Rate: 75.4%

@wfealdel wfealdel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed PR #1613. One minor finding (rfind scoping), one consistency question, and one architectural observation. See inline comments for details.

Comment thread src/aap_eda/services/project/scm.py Outdated
Comment thread src/aap_eda/services/project/scm.py Outdated
Comment thread src/aap_eda/services/project/scm.py Outdated

@ptoscano ptoscano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than "fix the regexp", I think the code needs to use better the ansible_runner module; in particular, the Runner object returned by ansible_runner.run() seem to provide already all the needed pieces, without any need to manual parsing:
https://docs.ansible.com/projects/runner/en/latest/python_interface/

My suggestion here is to create a different PR with the better usage of the ansible_runner API.

@jcraiglo1
jcraiglo1 marked this pull request as draft July 15, 2026 13:01
@jcraiglo1

Copy link
Copy Markdown
Contributor Author

/run-e2e

@jcraiglo1

Copy link
Copy Markdown
Contributor Author

/run-atf-tests

@aap-pde-ci-bot

Copy link
Copy Markdown

❌ Test Results - FAILED

Summary

Metric Count
Total Tests 66
✅ Passed 49
❌ Failed 1
⚠️ Errors 0
⏭️ Skipped 16
⏱️ Duration 235.72s

Pass Rate: 74.2%

❌ Failed Tests

Test Class
test_activation_reaches_completed_status opt.test-suite.tests.api.test_activations.TestEdaActivations

@jcraiglo1
jcraiglo1 marked this pull request as ready for review July 16, 2026 21:40

@ptoscano ptoscano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates, in general it is a good improvement. I left some more notes, I should had reviewed it fully previously, sorry.

In addition to the notes, this PR needs:

  • to be rebased
  • a PR description that explains better what is being done, and why

Comment thread src/aap_eda/services/project/scm.py
Comment thread tests/unit/test_scm.py Outdated
Comment thread tests/unit/test_scm.py
Comment thread tests/unit/test_scm.py
Comment thread tests/unit/test_scm.py Outdated
Comment thread tests/unit/test_scm.py Outdated
Comment thread tests/unit/test_scm.py Outdated
@sonarqubecloud

Copy link
Copy Markdown

@jcraiglo1
jcraiglo1 requested a review from ptoscano July 17, 2026 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants