Enable Packit integration in pull requests #143
Conversation
alexsaezm
force-pushed
the
main
branch
2 times, most recently
from
20ef3a1 to
e216242
Compare
|
I fixed a small typo in the commit message, nothing relevant |
|
The packit jobs aren't visible here, so I guess Packit service access isn't enabled yet for this repo? Either that, or we need to check with packit team to get this repo approved. |
Yes, it's not enabled for this repository because I didn't want to enable it unilaterally without talking to the rest of the team. I spoke with them and I approved my fork, so I can do all the testing. But yeah, this is a must if the rest of the team want to enable this feature. |
This feature adds support for Packit and the Packit bot. It adds two files: - .packit.yaml: Defines the tasks. It can be expanded later in case other features are attractive. - scripts/packit.sh: Variables can't be passed due to YAML limitations, and some commands with complex structures fail. Hence the script. The features inside the script are complex to use outside the Packit system; instead of having two scripts, two different functionalities are inside this file. How it works: When a PR is open or has changed, the bot packit-as-a-service will detect them. Then, three stages are run: - create-archive: this stage will run scripts/packit.sh using a create-archive argument that triggers git archive. Packit default functionality in this stage fails. By default, it thinks we are trying to substitute the Go upstream tarball. - post-upstream-clone: this stage will clone the ELN branch inside a new folder named .packit_rpm. - fix-spec-file: Several actions performed by scripts/packit.sh are executed here. It detects the version targeted in the PR, fakes the spec file previously downloaded in the second stage, and removes the patches that Fedora carries because those changes will likely conflict with the changes in the pull request.
.packit.yaml
Outdated
| - src: .packit_rpm/golang.spec | ||
| dest: golang.spec |
There was a problem hiding this comment.
The rpm spec file will always be synced by packit. We don't need lines 19-20.
| - fedora-eln-aarch64 | ||
| - fedora-eln-ppc64le | ||
| - fedora-eln-s390x | ||
| - fedora-eln-x86_64 |
There was a problem hiding this comment.
I would recommend adding centos-stream-$releasever-$arch and epel-$releasever-$arch targets as well as we'll also be shipping over there.
There was a problem hiding this comment.
quick note, currently we don't have epel-10 targets on copr, so it'll only be epel-9. But centos stream can have both 9 and 10.
There was a problem hiding this comment.
I am concern that will increase the complexity as CentOS Stream and ELN spec files might be potentially different at some times, but we should try on CentOS Stream too for sure.
Not sure about EPEL, why is necessary? There is no golang spec file for EPEL 8/9.
There was a problem hiding this comment.
I am concern that will increase the complexity as CentOS Stream and ELN spec files might be potentially different at some times, but we should try on CentOS Stream too for sure.
There are maybe two approaches you can try:
- Make both ELN and CentOS Stream 9/10 buildable using a single spec file.
- Include separate spec files for both ELN and CentOS Stream and mention them like this . You can replace you existing
specfile_pathline with that and use the separate paths for each of fedora and centos-stream.
I guess we could leave that for a followup PR.
Not sure about EPEL, why is necessary? There is no golang spec file for EPEL 8/9.
This could instead be a rhel-9-$arch copr target as well.
BTW, a quirk about copr is that on a rhel / centos-stream env if you run dnf copr enable foo/bar it will by default enable the epel target if it exists. And the epel copr targets are rhel + epel envs. So, if you wanna check buildability on proper rhel, you could use either of rhel- or epel- and get the same results. Just that the copr quirk might make using epel a little more convenient for testing etc.
This could also be left for a followup.
Packit always syncs the specfile
| # We need to tell Packit which files to sync from the upstream repository. | ||
| files_to_sync: | ||
| - .packit.yaml | ||
| - ./scripts/packit.sh |
There was a problem hiding this comment.
Do you need the changes from this script reflected in the final downstream rpm as well?
There was a problem hiding this comment.
You mean if they will be included in the CentOS Stream packages? They are only for this GitHub project, I don't think we need them anywhere else.
There was a problem hiding this comment.
In that case, line 18 can be removed too.
There was a problem hiding this comment.
Thanks a ton for the work so far @alexsaezm !
But yeah, this is a must if the rest of the team want to enable this feature.
can we get that enabled please? We would need that before this can be sufficiently reviewed.
This feature adds support for Packit and the Packit bot. This will allow PRs to be tested in multiple architectures by using Fedora ELN and Fedora COPR as environment.
It adds two files:
How it works:
When a PR is open or has changed, the bot packit-as-a-service will detect them. Then, three stages are run: