Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sanitize #408

Merged
merged 5 commits into from
Feb 4, 2025
Merged

Fix sanitize #408

merged 5 commits into from
Feb 4, 2025

Conversation

dimitry-ishenko
Copy link
Collaborator

@dimitry-ishenko dimitry-ishenko commented Feb 2, 2025

Description

Fixes sanitize_input command used with the --api and --cmd options.

Issue reference: Closes #324.

Implementation Details

Provide a detailed description of the implementation. Include the following:

  • Key changes introduced by this PR

Replaced sanitize_input function with simplified version called sanitize. Sanitation is only done on the first argument (which is a function name in case of --api, or a command name in case of --cmd).

UPDATE: The final commit fixes some flawed tests, whose failure was masked by explicit exit 0 statements in armbian-config.

  • Justification for the changes

See #324 for more details.

  • Confirmation that no new external dependencies or modules have been introduced

Testing Procedure

Describe the tests you ran to verify your changes. Provide relevant details about your test configuration.

./bin/armbian-config --cmd DEV-001 # FAILED
./bin/armbian-config --cmd DEV001
./bin/armbian-config --cmd DEV002

./bin/armbian-config --api pkg-install vim-airline # FAILED
./bin/armbian-config --api pkg_install vim-airline
./bin/armbian-config --api pkg_remove vim-airline

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have ensured that my changes do not introduce new warnings or errors
  • No new external dependencies are included
  • Changes have been tested and verified
  • I have included necessary metadata in the code, including associative arrays

This part will never be executed as line 27 already checks for non-root
user and relaunches armbian-config with sudo.
This makes all `exit 0` statements unnecessary and simplifies control
flow.
@github-actions github-actions bot added the size/medium PR with more then 50 and less then 250 lines label Feb 2, 2025
@dimitry-ishenko
Copy link
Collaborator Author

dimitry-ishenko commented Feb 3, 2025

OK, there are two failed tests -- CON005 and MON001...

The problem is that these tests should fail by design. Specifically, they call module_blah uninstall and then check the status with module_blah status, which will return exit code 1 by design. However, the failure was masked by the explicit exit 0 statement inside armbian-config. In other words, this command would never fail, even if the module was not installed:

./bin/armbian-config --api module_haos status >/dev/null 2>&1 || echo Not installed

And this would output success:

./bin/armbian-config --api WTF MATE >/dev/null 2>&1 && echo SUCCESS
SUCCESS

@github-actions github-actions bot added Documentation Documentation changes or additions Unit Tests size/large PR with 250 lines or more and removed size/medium PR with more then 50 and less then 250 lines labels Feb 3, 2025
@dimitry-ishenko dimitry-ishenko force-pushed the fix-sanitize branch 2 times, most recently from 74e58c4 to f8101df Compare February 3, 2025 16:32
@dimitry-ishenko dimitry-ishenko added the Ready to merge Reviewed, tested and ready for merge label Feb 3, 2025
@igorpecovnik igorpecovnik merged commit 36735c0 into main Feb 4, 2025
44 of 45 checks passed
@igorpecovnik igorpecovnik deleted the fix-sanitize branch February 4, 2025 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Documentation changes or additions Ready to merge Reviewed, tested and ready for merge size/large PR with 250 lines or more Unit Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: sanitize_input does not work as expected
2 participants