Skip to content

fix: sonar issues: Add defaults to optional args and small general fixes#1620

Open
jcraiglo1 wants to merge 1 commit into
mainfrom
sonar/S8396-optional-defaults
Open

fix: sonar issues: Add defaults to optional args and small general fixes#1620
jcraiglo1 wants to merge 1 commit into
mainfrom
sonar/S8396-optional-defaults

Conversation

@jcraiglo1

@jcraiglo1 jcraiglo1 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

https://redhat.atlassian.net/browse/AAP-77394

One of multiple PRs addressing sonar quality gate issues. Sonar Cloud flags optional arguments without defaults. This adds defaults of None to truly optional arguments. This PR also addresses other small issues that do not fit neatly into other categories.

Attached is a document describing the actions taken here.

sonarcloud-remediation-report.md

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of concatenated PEM credentials during validation.
    • Improved error reporting for invalid registry host names.
    • Updated credential defaults to apply only to supported choice lists.
    • Preserved permission checks when assigning superuser status.
    • Continued safely redacting credentials from proxy URLs.
  • Compatibility

    • WebSocket messages now accept omitted optional fields more reliably.
    • Event timestamps and vault password labels can be omitted without validation errors.

@jcraiglo1
jcraiglo1 requested a review from a team as a code owner July 16, 2026 16:38
@jcraiglo1
jcraiglo1 marked this pull request as draft July 16, 2026 16:39
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

The changes update optional message defaults and annotations, adjust PEM and credential validation logic, and simplify existing serializer conditions. Registry data gains an explicit string annotation.

Changes

Validation and message contract updates

Layer / File(s) Summary
Optional message field contracts
src/aap_eda/wsapi/messages.py
ActionMessage and VaultPassword.label optional fields now default to None; AnsibleEventMessage.run_at is typed as Optional[str].
Utility parsing and validation behavior
src/aap_eda/core/utils/awx.py, src/aap_eda/core/utils/credentials.py
PEM remainder matching is greedy, non-validation hostname exceptions are explicitly re-raised, and default choice handling requires a list.
API condition and type annotations
src/aap_eda/api/serializers/project.py, src/aap_eda/api/serializers/user.py, src/aap_eda/conf/registry.py
Proxy and superuser conditions are consolidated, and RegistryData.category_slug is explicitly annotated as a string.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 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 is concise and points to the main change: adding defaults to optional args with minor cleanup.
Description check ✅ Passed The description covers the issue link, what changed, why it was needed, and how it addresses the SonarCloud findings.
✨ 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/S8396-optional-defaults

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.

@jcraiglo1 jcraiglo1 changed the title fix: sonar issues: Add defaults to optional args fix: sonar issues: Add defaults to optional args and small general fixes Jul 16, 2026
@jcraiglo1

Copy link
Copy Markdown
Contributor Author

/run-e2e

@jcraiglo1

Copy link
Copy Markdown
Contributor Author

/run-atf-tests

@codecov-commenter

codecov-commenter commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.76%. Comparing base (51086aa) to head (a0a88bb).

Files with missing lines Patch % Lines
src/aap_eda/core/utils/credentials.py 33.33% 2 Missing ⚠️
@@           Coverage Diff           @@
##             main    #1620   +/-   ##
=======================================
  Coverage   92.75%   92.76%           
=======================================
  Files         244      244           
  Lines       11346    11344    -2     
=======================================
- Hits        10524    10523    -1     
+ Misses        822      821    -1     
Flag Coverage Δ
unit-int-tests-3.11 92.76% <88.88%> (+<0.01%) ⬆️
unit-int-tests-3.12 92.76% <88.88%> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
src/aap_eda/api/serializers/project.py 93.45% <100.00%> (ø)
src/aap_eda/api/serializers/user.py 97.56% <100.00%> (-0.03%) ⬇️
src/aap_eda/conf/registry.py 96.82% <100.00%> (ø)
src/aap_eda/core/utils/awx.py 58.51% <ø> (ø)
src/aap_eda/wsapi/messages.py 100.00% <100.00%> (ø)
src/aap_eda/core/utils/credentials.py 94.90% <33.33%> (+0.17%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sonarqubecloud

Copy link
Copy Markdown

@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 206.75s

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 17, 2026 14:33
@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 33
❌ Failed 0
⚠️ Errors 17
⏭️ Skipped 16
⏱️ Duration 50.50s

Pass Rate: 50.0%

❌ Failed Tests

Test Class
test_activation_reaches_completed_status opt.test-suite.tests.api.test_activations.TestEdaActivations
test_delete_decision_environment_with_force_flag opt.test-suite.tests.api.test_decision_environments.TestDecisionEnvironments
test_list_projects[bulk_projects0] opt.test-suite.tests.api.test_projects.TestProjects
test_create_project_duplicated opt.test-suite.tests.api.test_projects.TestProjects
test_read_project opt.test-suite.tests.api.test_projects.TestProjects
test_delete_project opt.test-suite.tests.api.test_projects.TestProjects
test_update_project opt.test-suite.tests.api.test_projects.TestProjects
test_update_project_url opt.test-suite.tests.api.test_projects.TestProjects
test_update_project_bad_foreign_keys opt.test-suite.tests.api.test_projects.TestProjects
test_update_project_null_credential opt.test-suite.tests.api.test_projects.TestProjects
test_update_project_no_changes opt.test-suite.tests.api.test_projects.TestProjects
test_update_project_duplicated_name opt.test-suite.tests.api.test_projects.TestProjects
test_create_update_project_empty_name opt.test-suite.tests.api.test_projects.TestProjects
test_filter_project_name[bulk_projects0] opt.test-suite.tests.api.test_projects.TestProjects
test_filter_project_name[bulk_projects1] opt.test-suite.tests.api.test_projects.TestProjects
test_filter_project_url[bulk_projects0] opt.test-suite.tests.api.test_projects.TestProjects
test_change_organization_of_project_to_invalid_value opt.test-suite.tests.api.test_projects.TestProjects

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.

3 participants