Skip to content

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented Dec 13, 2024

See individual commits for details.

Summary by Sourcery

Tests:

  • Run system tests using bats.

Copy link

sourcery-ai bot commented Dec 13, 2024

Reviewer's Guide by Sourcery

This pull request updates the testing and gating configurations to include CentOS Stream and OSCI. It adds support for CentOS Stream 9 and 10, Fedora ELN, as well as aarch64 architecture in the Copr build jobs. The test matrix is updated to include testing on CentOS Stream 9 and 10 for stable and testing releases using Bodhi and OSCI.

Flow diagram: Updated gating process with Bodhi and OSCI

flowchart LR
    Build[Build] --> Tests[Tests]
    Tests --> Decision{Gating Decision}
    Decision -->|Pass| Push[Push to Repository]
    Decision -->|Fail| Reject[Reject Update]

    subgraph Gating["Gating Process"]
        direction TB
        T1["fedora-ci.koji-build.tier0.functional"] --> GD["Gating Decision"]

        subgraph Contexts["Decision Contexts"]
            DC1["bodhi_update_push_stable"]
            DC2["bodhi_update_push_testing"]
        end

        Contexts --> GD
    end

    style Gating fill:#f5f5f5,stroke:#333,stroke-width:2px
Loading

File-Level Changes

Change Details Files
The test script was simplified to only run system tests.
  • Removed the logic to determine the test type.
  • Removed the logic to download and install podman and podman-tests rpms.
  • Removed the logic to run e2e tests.
  • The script now only runs system tests using bats.
test/podman-tests.sh
The packit configuration was updated to include aarch64 architecture and new Fedora and CentOS Stream targets.
  • Added aarch64 architecture to the fedora-all target.
  • Added aarch64 architecture to the fedora-eln target.
  • Added aarch64 architecture to the centos-stream-9 and centos-stream-10 targets.
  • Renamed the fedora-all target to fedora-all-x86_64 and added a new fedora-all-aarch64 target.
.packit.yaml
The gating configuration was updated to include testing releases.
  • Added 'bodhi_update_push_testing' to the decision_context.
rpm/gating.yaml
The test plan and makefile were removed.
  • Removed the test plan file.
  • Removed the test makefile.
plans/main.fmf
test/main.fmf
test/Makefile

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!
  • Generate a plan of action for an issue: Comment @sourcery-ai plan on
    an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@lsm5 lsm5 force-pushed the c9s-update-gating branch 2 times, most recently from 9e337a3 to 7441a24 Compare December 16, 2024 08:05
@lsm5 lsm5 force-pushed the c9s-update-gating branch 3 times, most recently from c647aae to cbfb6db Compare December 26, 2024 09:46
@lsm5 lsm5 mentioned this pull request Dec 26, 2024
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@lsm5 lsm5 force-pushed the c9s-update-gating branch from c9a5264 to cbfb6db Compare December 31, 2024 11:41
@lsm5 lsm5 marked this pull request as ready for review December 31, 2024 12:14
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @lsm5 - I've reviewed your changes - here's some feedback:

Overall Comments:

  • The removal of e2e tests from podman-tests.sh needs explanation. Was this intentional? If so, please provide rationale for removing this test coverage.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@lsm5
Copy link
Member Author

lsm5 commented Dec 31, 2024

Hey @lsm5 - I've reviewed your changes - here's some feedback:

Overall Comments:

  • The removal of e2e tests from podman-tests.sh needs explanation. Was this intentional? If so, please provide rationale for removing this test coverage.

Intentional. Running e2e tests by fetching sources with dnf download ... is a PITA on CentOS and imho not worth the hassle. We are running system tests anyway. FWIW, bodhi updates for the podman package only run its system tests.

If we absolutely need to run podman e2e tests, we should fetch sources using a podman-src / podman-devel subpackage.

@lsm5 lsm5 marked this pull request as draft December 31, 2024 13:20
@lsm5 lsm5 force-pushed the c9s-update-gating branch 3 times, most recently from e3d3645 to 7964f54 Compare December 31, 2024 14:22
@lsm5 lsm5 force-pushed the c9s-update-gating branch from 7964f54 to cd094fd Compare January 21, 2025 10:38
Copy link

Tests failed. @containers/packit-build please check.

@lsm5 lsm5 force-pushed the c9s-update-gating branch 5 times, most recently from 7f146d2 to 5817cf0 Compare January 22, 2025 11:09
@lsm5 lsm5 force-pushed the c9s-update-gating branch from 5817cf0 to 1d6b284 Compare January 28, 2025 11:17
@lsm5 lsm5 marked this pull request as ready for review January 28, 2025 11:59
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @lsm5 - I've reviewed your changes - here's some feedback:

Overall Comments:

  • The podman-tests.sh changes remove significant functionality around RPM source handling. Please explain the rationale for this simplification and where this functionality has been moved to.
  • The PR removes test/Makefile without explanation. Please provide context for this removal and confirm if this functionality has been migrated elsewhere.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

adjust+:
enabled: false
when: initiator == packit
- when: distro == centos-stream or distro == rhel
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of /upstream and /downstream is intentional as we can run the same set of tests across the board.

lsm5 added 3 commits January 28, 2025 17:36
`dnf download` on CentOS Stream ends up downloading all rpm versions of
a package from all available repos instead of only the latest rpm. This
leads to more (unnecessary) complications in the test script.

Things are a lot simpler if we directly test using the `podman-tests`
package. This means we'll have to disable podman e2e tests and only do
system tests but that shouldn't be too big a problem.

A better way to run podman e2e tests would be by creating a `podman-src`
rpm subpackage that'll install all the rpm sources.

The same set of tests can be run across all environments so there's no
need to maintain separate plans for upstream and downstream.

Signed-off-by: Lokesh Mandvekar <lsm5@fedoraproject.org>
container-selinux maybe noarch but it would help to have aarch64
visibility to ensure everything works, especially RE: podman.

Signed-off-by: Lokesh Mandvekar <lsm5@fedoraproject.org>
OSCI will gate on TMT tests for CentOS Stream.

Bodhi will gate on pushes for both testing and stable.

Signed-off-by: Lokesh Mandvekar <lsm5@fedoraproject.org>
@lsm5 lsm5 force-pushed the c9s-update-gating branch from 1d6b284 to e8677a4 Compare January 28, 2025 12:07
@lsm5
Copy link
Member Author

lsm5 commented Jan 28, 2025

Hey @lsm5 - I've reviewed your changes - here's some feedback:

Overall Comments:

  • The podman-tests.sh changes remove significant functionality around RPM source handling. Please explain the rationale for this simplification and where this functionality has been moved to.

Intentional. Running e2e tests by fetching sources with dnf download ... is a PITA on CentOS and imho not worth the hassle. We are running system tests anyway. FWIW, bodhi updates for the podman package only run its system tests.

If we absolutely need to run podman e2e tests, we should fetch sources using a podman-src / podman-devel subpackage.

  • The PR removes test/Makefile without explanation. Please provide context for this removal and confirm if this functionality has been migrated elsewhere.

No need for this Makefile. We're triggering the test script directly.

@rhatdan @inknos PTAL

Comment on lines +15 to +18
COPR_REPO_FILE="/etc/yum.repos.d/*podman-next*.repo"
if compgen -G $COPR_REPO_FILE > /dev/null; then
sed -i -n '/^priority=/!p;$apriority=1' $COPR_REPO_FILE
fi
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would something like this work? I think it's more readable

dnf config-manager --save --setopt="podman-next*.priority=1"

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I just pulled it out from memory and I am not sure if it's going to be the same for dnf4/dnf5. I should verify that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things have changed quite a bit between dnf4 and dnf5. So, unless we have something that works across the board, I'd prefer to keep it this way. I pulled this from packit project's own TMT config btw.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, this actually changes from dnf4/dnf5. I just verified that and in dnf5 it would be

dnf config-manager setopt "copr:copr.fedorainfracloud.org:rhcontainerbot:podman-next.priority=1"

which would add few lines in

/etc/dnf/repos.override.d/99-config_manager.repo

[copr:copr.fedorainfracloud.org:rhcontainerbot:podman-next]
priority=1

different command, different file: I am good with sed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so LGTM!

@lsm5
Copy link
Member Author

lsm5 commented Jan 30, 2025

Merging..

@lsm5 lsm5 merged commit b27a1d0 into containers:main Jan 30, 2025
24 checks passed
@lsm5 lsm5 deleted the c9s-update-gating branch January 30, 2025 15:54
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.

2 participants