Skip to content

Conversation

henrywang
Copy link
Collaborator

PR #1642 merged, so the packit-reboot.yml ansible playbook download URL should be moved to bootc-dev/bootc now.

Signed-off-by: Xiaofeng Wang <henrywangxf@me.com>
@bootc-bot bootc-bot bot requested a review from jeckersb October 3, 2025 02:21
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly updates the download URL for the packit-reboot.yml ansible playbook to point to the main bootc-dev/bootc repository. My review includes one suggestion to improve the stability of this test dependency by pinning the URL to a specific commit hash instead of the main branch. This will make the tests more reliable and prevent unexpected failures due to upstream changes.

- how: ansible
playbook:
- https://github.com/henrywang/bootc/raw/refs/heads/gating/hack/packit-reboot.yml
- https://github.com/bootc-dev/bootc/raw/refs/heads/main/hack/packit-reboot.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

While this change correctly points to the canonical repository, fetching the playbook from the main branch can cause test instability. If the remote packit-reboot.yml file changes, this test may break unexpectedly. For improved reliability and reproducible builds, it's best practice to pin the URL to a specific commit SHA instead of a branch name.

For example, you could use a URL like:
https://github.com/bootc-dev/bootc/raw/<full-commit-hash>/hack/packit-reboot.yml

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a really simple playbook for reboot, no additional code included. So no need to pin to commit.

Copy link
Contributor

@jmarrero jmarrero left a comment

Choose a reason for hiding this comment

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

lgtm

@henrywang henrywang merged commit 3fd43a7 into bootc-dev:main Oct 3, 2025
34 of 36 checks passed
@henrywang henrywang deleted the fix_ansible branch October 3, 2025 13:22
- how: ansible
playbook:
- https://github.com/henrywang/bootc/raw/refs/heads/gating/hack/packit-reboot.yml
- https://github.com/bootc-dev/bootc/raw/refs/heads/main/hack/packit-reboot.yml
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait but can't we just specify running the playbook from the local git checkout?

Copy link
Collaborator Author

@henrywang henrywang Oct 3, 2025

Choose a reason for hiding this comment

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

Two reasons:

  1. According to https://tmt.readthedocs.io/en/stable/plugins/prepare.html#ansible, I can't trace the git checkout folder. Very sad thing for me.
  2. The Fedora CI and OSCI gating does not have local git. I need extract .src.rpm. And it does not work with ansible prepare.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And I'm working on another PR to use .src.rpm instead of local git checkout. Just for working with dist-git gating test.

Copy link
Collaborator

@cgwalters cgwalters Oct 3, 2025

Choose a reason for hiding this comment

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

I can't trace the git checkout folder. Very sad thing for me.

What do you mean "trace"? Oh...are you getting hit by

# Move the tmt bits to a subdirectory to work around https://github.com/teemtee/tmt/issues/4062
?
We can add hack/ there too.

The Fedora CI and OSCI gating does not have local git. I need extract .src.rpm. And it does not work with ansible prepare.

Hmm. I often go back to referencing https://cockpit-project.org/blog/fmf-unified-testing.html

Though it looks like things have changed there for cockpit nowadays....

I also looked at https://src.fedoraproject.org/rpms/dnf5/blob/rawhide/f/.packit.yaml#_85 which is interesting.

Anyways I think the idea here is we should definitely be able to store this in a git repo directly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The dnf5 fedora dist-git does not have gating test for release PR, like https://src.fedoraproject.org/rpms/dnf5/pull-request/102.

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.

3 participants