Add aide-helpers library#58
Open
msafarik wants to merge 8 commits into
Open
Conversation
Signed-off-by: Marek Safarik <msafarik@redhat.com>
Reviewer's GuideIntroduce a shared Beakerlib-based aide-helpers library for AIDE tests, centralizing database path resolution, initialization/check helpers, config utilities, and test directory management; update existing tests to consume the shared helpers instead of duplicating logic. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
aideLibraryLoaded, automatically runningyum -y install aideinside a helper library can be surprising and may fail on non-yum systems; consider relying on existingrlAssertRpm/test setup instead of performing package installation in the library. - The
sedpatterns inaideSetupConfigassume that@@define DBDIRand@@define LOGDIRlines are present in the source config; if they are missing or formatted differently, the function silently leaves them unchanged—consider handling that case explicitly or returning an error.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `aideLibraryLoaded`, automatically running `yum -y install aide` inside a helper library can be surprising and may fail on non-yum systems; consider relying on existing `rlAssertRpm`/test setup instead of performing package installation in the library.
- The `sed` patterns in `aideSetupConfig` assume that `@@define DBDIR` and `@@define LOGDIR` lines are present in the source config; if they are missing or formatted differently, the function silently leaves them unchanged—consider handling that case explicitly or returning an error.
## Individual Comments
### Comment 1
<location path="Library/aide-helpers/lib.sh" line_range="86" />
<code_context>
+
+aideGetDbPaths() {
+ local CONF="${1:-$__INTERNAL_aideConfDefault}"
+ DBDIR=$(sed -n -e 's/@@define DBDIR \([a-z/]\+\)/\1/p' "$CONF")
+ if rlIsRHELLike "=<9.7"; then
+ DB=$(grep "^database=" "$CONF" | cut -d/ -f2-)
</code_context>
<issue_to_address>
**issue (bug_risk):** DBDIR extraction regex is overly restrictive and may fail for common directory names.
The current pattern `\([a-z/]\+\)` will fail for paths containing digits, underscores, dashes, uppercase letters, etc., leaving `DBDIR` empty and potentially breaking downstream logic. Please broaden the match (for example, using `[^ ]\+` or `\(.\+\)` with suitable anchoring) so it reliably captures any valid directory path used in `@@define DBDIR`.
</issue_to_address>
### Comment 2
<location path="Library/aide-helpers/lib.sh" line_range="278-279" />
<code_context>
+ local DEST="$1"
+ local SRC="${2:-$__INTERNAL_aideConfDefault}"
+ rlRun "cp $SRC $DEST/aide.conf"
+ rlRun "sed -i 's#^@@define DBDIR.*#@@define DBDIR ${DEST}/db#' $DEST/aide.conf"
+ rlRun "sed -i 's#^@@define LOGDIR.*#@@define LOGDIR ${DEST}/log#' $DEST/aide.conf"
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Using DEST directly in the sed replacement can break when DEST contains `#` or other special characters.
Since `#` is the sed delimiter, any `#` in `${DEST}` will break the command (prematurely ending the pattern or causing a syntax error). This is fragile if tests or callers pass arbitrary paths. Consider using a delimiter less likely to appear in paths (e.g. `|`) or escaping `${DEST}` before inserting it into the sed expression.
</issue_to_address>
### Comment 3
<location path="Library/aide-helpers/lib.sh" line_range="395-396" />
<code_context>
+
+aideCreateTestDir() {
+ local DIR="${1:-/var/aide-testing-dir}"
+ mkdir -p "$DIR/db" "$DIR/log" "$DIR/data"
+ echo "$DIR"
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** aideCreateTestDir does not use rlRun or check for failures when creating directories.
Other helpers wrap side-effecting operations (cp, sed, rm) in `rlRun` for logging and error handling. Here `mkdir -p` runs directly, so permission or other errors are silent while the function still echoes the directory. Please use `rlRun "mkdir -p ..."` so failures are reported to beakerlib and callers don’t see a false success.
Suggested implementation:
```
aideCreateTestDir() {
local DIR="${1:-/var/aide-testing-dir}"
rlRun "mkdir -p \"$DIR/db\" \"$DIR/log\" \"$DIR/data\"" 0 \
"Create AIDE test directories in $DIR" || return $?
echo "$DIR"
}
```
This change assumes `beakerlib.sh` is sourced before `aideCreateTestDir` is ever called so that `rlRun` is available. If it is not, you should either:
1. Move the `. /usr/share/beakerlib/beakerlib.sh || exit 1` line above the function definition, or
2. Ensure the library is sourced by callers before invoking `aideCreateTestDir`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| aideGetDbPaths() { | ||
| local CONF="${1:-$__INTERNAL_aideConfDefault}" | ||
| DBDIR=$(sed -n -e 's/@@define DBDIR \([a-z/]\+\)/\1/p' "$CONF") |
There was a problem hiding this comment.
issue (bug_risk): DBDIR extraction regex is overly restrictive and may fail for common directory names.
The current pattern \([a-z/]\+\) will fail for paths containing digits, underscores, dashes, uppercase letters, etc., leaving DBDIR empty and potentially breaking downstream logic. Please broaden the match (for example, using [^ ]\+ or \(.\+\) with suitable anchoring) so it reliably captures any valid directory path used in @@define DBDIR.
added 3 commits
July 20, 2026 12:08
Signed-off-by: Marek Safarik <msafarik@redhat.com>
Contributor
Author
Contributor
|
Why is failing that CI for Rawhide? @msafarik |
Contributor
|
Should be cover more than 3 tests, also should cover tests in downstream, please extend it. @msafarik |
Koncpa
requested changes
Jul 21, 2026
Koncpa
left a comment
Koncpa
left a comment
Contributor
There was a problem hiding this comment.
Overall this library should be applied for all tests (upstream/downstream), I don't think that cover just three of them, also should be properly tested, not only via CI plan . Structure of function should be different, there should be one configuration function. Also creating tmp dir could be replaced by beakerlib general function.
| @@ -32,27 +32,9 @@ PACKAGE="aide" | |||
| AIDE_CONF="/etc/aide.conf" | |||
Contributor
There was a problem hiding this comment.
This test it's no ran in CI, should prepare TF where is executed for correctnes of changes, please schedule it.
|
|
||
| =cut | ||
|
|
||
| aideCreateTestDir() { |
Contributor
There was a problem hiding this comment.
Could be used temporary dir from base beakerlib library? And adjust in all tests?
|
|
||
| =cut | ||
|
|
||
| aideCleanupTestDir() { |
Contributor
There was a problem hiding this comment.
When would be used creating dir from base beakerlib library, this function it's not needed, but also need to be replaced in code.
|
|
||
| =cut | ||
|
|
||
| aideSetupConfig() { |
Contributor
There was a problem hiding this comment.
Couldnt be one function which prepare whole configuration for aide testing?
| Removes everything after the "Next decide what directories/files" | ||
| comment. | ||
|
|
||
| aideStripDefaultRules AIDE_CONF |
Contributor
There was a problem hiding this comment.
Couldnt be one function which prepare whole configuration for aide testing?
|
|
||
| =cut | ||
|
|
||
| aideStripPaths() { |
Contributor
There was a problem hiding this comment.
Couldnt be one function which prepare whole configuration for aide testing?
|
|
||
| =cut | ||
|
|
||
| aideAddContentexGroup() { |
Contributor
There was a problem hiding this comment.
Couldnt be one function which prepare whole configuration for aide testing?
Contributor
Author
Contributor
Author
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extract duplicate functions (aideInit, aideCheck, aideGetDbPaths, config helpers) from tests into a shared Library/aide-helpers
Summary by Sourcery
Introduce a shared aide-helpers testing library and update existing aide tests to use it instead of duplicated inline helpers.
Enhancements:
Tests: