Make CI green again (exclude --tls-details podman tests)#2044
Make CI green again (exclude --tls-details podman tests)#2044kolyshkin wants to merge 3 commits intocontainers:mainfrom
Conversation
The -skip argument grew too big, also comments about why we exclude this or that became detached and redundant. Let's return to sanity. While at it, remove the redundant .* at the beginning and the end of regexp, and the redundant/mistaken * at the end of "podman exec 12*". Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
First, remove -progress flag, since ginkgo complains: > --progress is deprecated . The functionality provided by --progress > was confusing and is no longer needed. Use --show-node-events instead > to see node entry and exit events included in the timeline of failed and > verbose specs. Or you can run with -vv to always see all node events. Second, replace "-vv" with "-v --show-node-events" to be slightly less verbose. Third, remove "exclude_graphdriver_devicemapper" tag since it is no longer used (hooray to that). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Those were recently added in podman PR 28208 and are failing here. As most probably they have nothing to do with OCI runtime, and are failing, let's skip them. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request aims to stabilize the continuous integration (CI) pipeline by addressing failing Podman tests and improving the test execution script. It specifically excludes a newly introduced set of Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix the CI by excluding a failing test. It also includes a significant and beneficial refactoring of how skipped tests are managed in tests/podman/run-tests.sh, improving readability and maintainability. My review includes a critical point about a removed build tag that might reintroduce CI failures, and a suggestion to make the script more robust.
|
|
||
| ginkgo --focus='.*' --skip="$SKIP_REGEX" \ | ||
| -v --show-node-events --trace \ | ||
| -tags "seccomp ostree selinux" \ |
There was a problem hiding this comment.
The exclude_graphdriver_devicemapper build tag was removed. Since the test environment is configured to use the overlay storage driver (as seen on line 25), this tag is likely necessary to exclude tests specific to devicemapper that would otherwise run and fail. If its removal was unintentional, it should be restored to prevent CI failures.
| -tags "seccomp ostree selinux" \ | |
| -tags "seccomp ostree selinux exclude_graphdriver_devicemapper" \ |
There was a problem hiding this comment.
Too bad you're not reading commit messages 👎🏻
There was a problem hiding this comment.
Thank you for pointing that out. My analysis was based solely on the provided diff and the common practice of excluding devicemapper tests when overlay is the primary storage driver in the test environment. If the commit message clarifies that the exclude_graphdriver_devicemapper tag is no longer necessary for this specific test setup, then my concern is addressed.
| # Not related to runtime. | ||
| '--tls-details' | ||
| ) | ||
| SKIP_REGEX=$(IFS='|'; echo "${SKIP_TESTS[*]}") |
There was a problem hiding this comment.
Using echo to build the regex can be fragile. If any of the test names in SKIP_TESTS were, for example, -n, echo would interpret it as an option rather than a string to print, leading to an incorrect regex. Using printf and paste is a more robust way to join the array elements that is not susceptible to this issue.
| SKIP_REGEX=$(IFS='|'; echo "${SKIP_TESTS[*]}") | |
| SKIP_REGEX=$(printf '%s\n' "${SKIP_TESTS[@]}" | paste -sd '|') |
There was a problem hiding this comment.
I don't see such behavior on my system
[kir@kir-tp1 crun]$ IFS="|"; SKIP_TESTS=("-n" "-e" "\t\t\txxx\t\t\t"); echo "${SKIP_TESTS[*]}"
-n|-e|\t\t\txxx\t\t\tThis is because [*] (rather than [@] as you suggested) is used, and it concatenates all values into one long string, and it doesn't matter if it contains -n or something similar that can be interpreted as echo option. The only possible way is to have something like -n as the ONLY option, but this is highly unlikely here.
There was a problem hiding this comment.
You are correct that echo "${SKIP_TESTS[*]}" concatenates the array elements into a single string before echo processes it, which mitigates the specific issue of -n being interpreted as an option when it's the only argument.
However, the printf '%s\n' "${SKIP_TESTS[@]}" | paste -sd '|' approach remains more robust for general string manipulation. It explicitly prints each element as a separate string and then joins them, which is less susceptible to unexpected interpretations by echo if an array element were to contain other strings that echo might interpret as options (e.g., -e followed by a string, or other less common echo flags). While the current test names might not trigger such issues, using printf and paste provides a more predictable and safer way to construct the regex string from an array.
1 participant
tests/podman: exclude --tls-details tests
Those were recently added in podman PR 28208 and are failing here.
As most probably they have nothing to do with OCI runtime, and are
failing, let's skip them.
This PR also includes some cleanup of
tests/podman/run-tests.shwhichI forgot to submit earlier, so I'm shoving it here.