Add F41 tests #145
Add F41 tests #145
Conversation
|
/run tests |
|
/run tests |
|
Yeah, using our old workaround works. Not sure the feature works properly in Anaconda yet. I can always check upstream with the Anaconda devs. Thoughts @JasonN3 ? |
|
@coderabbitai review |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to several files related to the Flatpak environment setup and configuration. Key updates include the addition of version "41" in the GitHub Actions workflow, adjustments to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
test/vm/flatpak_fedora_repo_disabled.yml (1)
Line range hint 13-17: Consider enhancing error messages for better debugging.
The current implementation could benefit from more descriptive error handling.
Consider applying this improvement:
when: services_state['ansible_facts']['services']['flatpak-add-fedora-repos.service'] is defined
ansible.builtin.assert:
that:
- services_state['ansible_facts']['services']['flatpak-add-fedora-repos.service']['status'] == 'disabled'
- fail_msg: 'flatpak-add-fedora-repos.service is not disabled'
+ fail_msg: "flatpak-add-fedora-repos.service is in '{{ services_state['ansible_facts']['services']['flatpak-add-fedora-repos.service']['status'] }}' state instead of 'disabled'"
+ success_msg: "flatpak-add-fedora-repos.service is correctly disabled"🧰 Tools
🪛 yamllint
[warning] 16-16: wrong indentation: expected 6 but found 8
(indentation)
lorax_templates/flatpak_set_repo.tmpl (1)
1-8: Consider adding error handling for the configuration operations.
The template performs file operations (append/replace) without error handling. While Lorax might handle errors internally, it's worth verifying the behavior when:
- The target files don't exist
- The files are not writable
- The regex patterns don't match
Consider adding error handling or documentation about these edge cases.
test/iso/flatpak_repo_updated.sh (2)
3-7: Consider adding debug output.
The script silently exits for F41+ versions. Adding debug output would help with troubleshooting.
if [[ ${VERSION} -ge 41 ]]
then
+ echo "F41+ detected: Checking anaconda.conf configuration..."
grep "${_FLATPAK_REPO_URL}" mnt/install/etc/anaconda/conf.d/anaconda.conf > /dev/null
- exit $?
+ result=$?
+ if [ $result -eq 0 ]; then
+ echo "Successfully verified Flatpak configuration in anaconda.conf"
+ else
+ echo "Failed to find Flatpak configuration in anaconda.conf"
+ fi
+ exit $result
fiLine range hint 9-45: Make Python file path resolution more robust.
The current wildcard path mnt/install/usr/lib64/python*/site-packages/... could be fragile if multiple Python versions are present or if the directory structure changes.
Consider using find to locate the exact file:
-add_line=$(grep flatpak_manager.add_remote mnt/install/usr/lib64/python*/site-packages/pyanaconda/modules/payloads/payload/rpm_ostree/flatpak_installation.py)
+FLATPAK_PY=$(find mnt/install -name "flatpak_installation.py")
+if [ -z "${FLATPAK_PY}" ]; then
+ echo "Error: Could not find flatpak_installation.py"
+ exit 1
+fi
+add_line=$(grep flatpak_manager.add_remote "${FLATPAK_PY}")Apply similar changes to the second grep operation as well.
.github/workflows/build_vars.yml (1)
15-15: Remove trailing whitespace
There is trailing whitespace at the end of line 15.
- ],
+ ],🧰 Tools
🪛 yamllint
[error] 15-15: trailing spaces
(trailing-spaces)
flatpaks/Makefile (1)
17-17: Auto-installation of flatpak is a good addition, but needs more robustness.
While automatically installing flatpak is helpful for test environment setup, the current implementation could be more robust.
Consider these improvements:
- which flatpak &> /dev/null || dnf install -y flatpak
+ # Ensure flatpak is available for testing
+ if ! which flatpak &> /dev/null; then
+ echo "Installing flatpak package..."
+ if ! dnf install -y flatpak-1.* ; then
+ echo "Failed to install flatpak" >&2
+ exit 1
+ fi
+ fiThis version:
- Adds error handling for failed installation
- Pins the major version for consistency
- Improves logging for debugging
- Follows shell scripting best practices
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- .github/workflows/build_vars.yml (2 hunks)
- flatpaks/Makefile (1 hunks)
- lorax_templates/flatpak_set_repo.tmpl (1 hunks)
- test/iso/flatpak_repo_updated.sh (1 hunks)
- test/vm/flatpak_fedora_repo_disabled.yml (1 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/build_vars.yml
[error] 15-15: trailing spaces
(trailing-spaces)
🔇 Additional comments (8)
test/vm/flatpak_fedora_repo_disabled.yml (1)
13-13: LGTM! Good defensive programming practice.
Adding the existence check before asserting the service status is a good safeguard, especially important for testing F41 where Flatpak repository configurations might differ.
lorax_templates/flatpak_set_repo.tmpl (3)
2-4: Verify the new F41+ configuration approach.
The new approach for F41+ correctly addresses issue #144 by:
- Adding the missing "[Payload]" section header
- Setting the flatpak_remote configuration in the proper section
However, we should verify if this is the complete fix.
Let's validate the configuration structure:
#!/bin/bash
# Description: Verify the configuration structure for F41
# Expected: Find documentation or tests validating this configuration
# Search for related configuration documentation
rg -g "!*.pyc" -g "!*.pyo" "flatpak_remote.*anaconda\.conf"
# Search for related test cases
fd -e yml -e yaml -e py | xargs rg "flatpak_remote.*\[Payload\]"5-7: Ensure backward compatibility for pre-F41 versions.
The pre-F41 approach uses Python file modifications through regex replacements. While this maintains backward compatibility, there are potential risks:
- The regex patterns are quite specific and might break if the Python files change
- The file path pattern
/usr/lib64/python*/site-packages/...might match multiple Python versions
Let's validate the Python file modifications:
#!/bin/bash
# Description: Verify the Python file modifications
# Expected: Find the target Python files and their content structure
# Search for the target Python files
fd "flatpak_installation.py"
# Check the structure of flatpak_manager calls
rg -A 5 "class.*FlatpakManager|def add_remote|def replace_installed_refs_remote"1-1: Consider validating template parameters.
The template accepts parameters that are directly interpolated into configuration files and Python code. While Mako provides some XSS protection, it's recommended to add parameter validation.
Let's check how these parameters are used across the codebase:
test/iso/flatpak_repo_updated.sh (1)
3-7: Verify the path to anaconda.conf.
Let's verify if the path to anaconda.conf is consistent across the codebase.
✅ Verification successful
Let me check the content of these files to verify the path consistency.
The path to anaconda.conf is consistent and correctly used
The path /etc/anaconda/conf.d/anaconda.conf is consistently used across both files:
- In
test/iso/flatpak_repo_updated.shit's referenced asmnt/install/etc/anaconda/conf.d/anaconda.confwheremnt/installis the mount point for testing - In
lorax_templates/flatpak_set_repo.tmplit's referenced asetc/anaconda/conf.d/anaconda.confwhich is relative to the root directory
Both paths point to the same location, with the test script using a mount point prefix as expected in a test environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for all references to anaconda.conf to ensure path consistency
echo "Searching for anaconda.conf references:"
rg -l "anaconda.conf"
echo -e "\nChecking for any other potential config paths:"
rg -l "/etc/anaconda"
Length of output: 345
Script:
#!/bin/bash
# Check the content of both files to see how anaconda.conf is referenced
echo "Content of test/iso/flatpak_repo_updated.sh:"
cat test/iso/flatpak_repo_updated.sh
echo -e "\nContent of lorax_templates/flatpak_set_repo.tmpl:"
cat lorax_templates/flatpak_set_repo.tmpl
Length of output: 2336
.github/workflows/build_vars.yml (3)
13-14: LGTM: Version matrix updated correctly
The addition of version "41" aligns with the PR objective to incorporate F41 testing.
33-35: LGTM: Include section properly configured for F41
The F41 configuration correctly follows the same pattern as F40, using the fedora-bootc image.
18-18: Verify flatpak configuration changes across the codebase
The change from "flatpak_dir" to "flatpak_refs_dir" needs verification to ensure all dependent configurations are updated.
|
@coderabbitai review |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/build_vars.yml (1)
Line range hint
1-56: Consider adding documentation for build matrix optionsThe build matrix is growing in complexity with various configuration options. Consider adding inline documentation (as YAML comments) explaining the purpose and impact of each option in the matrix, especially for the newly added configurations.
Example addition:
BUILD_MATRIX: value: ' { + # Supported Fedora versions for testing "version": [ "39", "40", "41" ], + # Flatpak installation modes: + # - false: No Flatpak installation + # - flatpak_refs_dir: Install from refs directory + # - flatpak_refs: Install from specific refs "flatpaks": [ "false", "flatpak_refs_dir", "flatpak_refs" ],🧰 Tools
🪛 yamllint
[error] 12-12: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/build_vars.yml(1 hunks)test/iso/flatpak_repo_updated.sh(1 hunks)test/vm/flatpak_fedora_repo_disabled.yml(1 hunks)
🧰 Additional context used
🪛 yamllint
test/vm/flatpak_fedora_repo_disabled.yml
[warning] 16-16: wrong indentation: expected 6 but found 8
(indentation)
[error] 18-18: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (7)
test/vm/flatpak_fedora_repo_disabled.yml (2)
13-13: LGTM! Good defensive programming practice.
The added condition ensures the test doesn't fail incorrectly when the service is not present, making the test more robust.
Line range hint 1-18: Verify test coverage for F41.
Since this PR aims to add F41 tests, we should verify this test will run for F41 images.
✅ Verification successful
Looking at the GitHub workflows, I can see that F41 is included in the test matrix through the build_vars.yml workflow, which defines:
BUILD_MATRIX:
value: '
{
"version": [
"39",
"40",
"41"
],
...
}'This means the test flatpak_fedora_repo_disabled.yml will be executed for F41 as part of the test deployment workflow.
Test coverage for F41 is verified.
The test will run for F41 images as it's included in the test matrix versions, and the test deployment workflow executes all VM tests including this one for each version in the matrix.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if this test is included in F41 test suite
# Expected: Find references to this test file in F41-related test configurations
# Search for references to this test file
rg -l "flatpak_fedora_repo_disabled.yml"
# Search for F41 test configurations
rg -l "f41|41" --type yaml
Length of output: 69
Script:
#!/bin/bash
# Let's try a different approach to find test configurations and F41 references
# Find all YAML files in the test directory
fd -e yml -e yaml . test/
# Look for F41/f41 references with more context
rg -i "f41|fedora.*41" -A 5 -B 5
# Check if this test is referenced in any test suite configuration
rg -p "flatpak.*fedora.*repo" test/
Length of output: 703
Script:
#!/bin/bash
# Let's check the test configuration and inventory to understand test execution
# Check inventory file for VM configurations
cat ansible_inventory
# Look for test runner or test configuration files
fd -e yml -e yaml . -E test/vm/
# Check if there are any GitHub workflow files that might define test matrices
fd workflow .github/
Length of output: 160
Script:
#!/bin/bash
# Let's check GitHub workflows and action configuration
# Check action.yml for test configuration
cat action.yml
# Check workflow files
fd -e yml -e yaml . .github/workflows/ --exec cat {}
Length of output: 40152
🧰 Tools
🪛 yamllint
[warning] 16-16: wrong indentation: expected 6 but found 8
(indentation)
[error] 18-18: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/build_vars.yml (3)
13-15: LGTM: Version array updated correctly for F41 support
The addition of version "41" aligns with the PR objective to incorporate F41 testing support.
21-25: Verify integration of new image configurations
The addition of standardized image repository and name configurations looks good, but let's verify their integration with other configuration files.
#!/bin/bash
# Description: Check how these new image configurations are used across the codebase
echo "Checking for image repository references:"
rg "ghcr.io/ublue-os"
echo -e "\nChecking for base-main image references:"
rg "base-main"18-18: Verify the impact of flatpak_refs_dir rename
The change from "flatpak_dir" to "flatpak_refs_dir" might affect existing workflows. Let's verify this change is consistent across the codebase.
test/iso/flatpak_repo_updated.sh (2)
5-14: LGTM! Well-structured error handling.
The implementation properly handles errors by:
- Using a result variable to track multiple failures
- Providing clear error messages for each check
- Verifying both the section header and flatpak_remote option
3-15: Verify the correct section header for F41.
The code checks for [Payload] header, but there seems to be confusion about whether it should be [Flatpak] or [Payload]. Let's verify this with the Anaconda team.
This adds version 41 to the tests and adds the header for the
flatpak_remoteoption.Fixes #144
Summary by CodeRabbit
New Features
Bug Fixes
Documentation