-
Notifications
You must be signed in to change notification settings - Fork 332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replicate (mostly) buildbot workflow for commits into master #1225
Conversation
PACKAGE_MANAGER_INSTALL_yum="yum --setopt=skip_missing_names_on_install=False install -y" | ||
PACKAGE_MANAGER_REMOVE_yum="yum erase -y" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we drop CentOS 7 (EOL in a couple of months IIRC) we should replace yum with dnf (it replaced yum several years ago), I don't even have yum on my Fedora 39 boxes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like RedHat rebrands dnf
to yum
in RHEL 8: https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/configuring_basic_system_settings/managing-software-packages_configuring-basic-system-settings
Upstream documentation identifies the technology as DNF and the tool is referred to as DNF in the upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can fix this later, I guess.
As per fedora, in a default rawhide container yum
is still a symlink to dnf
, so I don't expect any problems with earlier releases either.
:~$ docker run --rm -it fedora:rawhide /usr/bin/yum --version
4.19.0
Installed: dnf-0:4.19.0-1.fc40.noarch at Mon Feb 19 08:10:37 2024
Built : Fedora Project at Thu Feb 8 16:33:42 2024
Installed: rpm-0:4.19.1.1-1.fc40.x86_64 at Mon Feb 19 08:10:37 2024
Built : Fedora Project at Wed Feb 7 15:55:53 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like RedHat rebrands
dnf
toyum
in RHEL 8: https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/configuring_basic_system_settings/managing-software-packages_configuring-basic-system-settingsUpstream documentation identifies the technology as DNF and the tool is referred to as DNF in the upstream.
Well, it's a version of yum that is compatible with dnf (and older yum).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can fix this later, I guess. As per fedora, in a default rawhide container
yum
is still a symlink todnf
, so I don't expect any problems with earlier releases either.:~$ docker run --rm -it fedora:rawhide /usr/bin/yum --version 4.19.0 Installed: dnf-0:4.19.0-1.fc40.noarch at Mon Feb 19 08:10:37 2024 Built : Fedora Project at Thu Feb 8 16:33:42 2024 Installed: rpm-0:4.19.1.1-1.fc40.x86_64 at Mon Feb 19 08:10:37 2024 Built : Fedora Project at Wed Feb 7 15:55:53 2024
Maybe I removed it...
$ yum
bash: yum: command not found
$ rpm -q yum
package yum is not installed
.github/workflows/ci-commit.yml
Outdated
- ubuntu-22.04 | ||
- ubuntu-23.10 | ||
- debian-11 | ||
- debian-12 | ||
- alpine-3.19 | ||
- rhel-9 | ||
- amazonlinux-2023 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are more package lists than what's listed here, is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and no: there is alpine-3.18
list which I failed to use because there is no alpine 3.18 image for self hosted runners. So this one can be removed.
At the same time, having general lists (debian
, alpine
, ubuntu
, rhel
) is intentional: one can install a package for all versions of an OS family.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rhel-8 also seems to be in the lists but not the workflow file...
Some observations
|
I agree with Andrew's comments above; LGTM once those are addressed. |
|
Let's rename back to |
.github/workflows/ci-pull.yml
Outdated
@@ -1,10 +1,8 @@ | |||
name: ci | |||
|
|||
on: | |||
workflow_dispatch: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this change? Things seem to work without it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows you to run a workflow manually on any branch. IMO it's useful and doesn't break anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allows us to kick off builds without actually making a change to the repository, manually. Can be really useful!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks for the explanation.
Then yeah, that should be done as a separate commit (it's not related to the other changes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file renaming part of this commit should be dropped.
If you keep the ci.yml change, then that should at least be a separate commit (with an explanation of why it's needed). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Assuming it works tho, as the job wasn't able to kick off within the PR, obviously.
If there are too many jobs, the long-running self hosted environments could run all tests together at once, or at least, matrix against every module-lang rather than module-lang-version. But that can always be optimized later.
I'm happy to help with some of those optimizations if needed, as well.
@arbourd Running too may tests is my main concern, really. I failed to find an easy way of pinning os-lang-version triplet while this will reduce the amount of jobs significantly. I'm happy to adopt any solution (maybe just hardcode it in?). |
Is it a such a good idea to create a job for every "$OS-$ARCH-$LANG"? I think if we're to reuse the buildbot approach, we should be fine with "$OS-$ARCH" and then run steps for different $LANGs... this will save us quite some overhead in spinning up the machines. |
.github/workflows/ci-commit.yml
Outdated
@@ -0,0 +1,345 @@ | |||
name: ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a more descriptive name...
.github/workflows/ci-commit.yml
Outdated
on: | ||
workflow_dispatch: | ||
push: | ||
branches: | ||
- master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do sort of question this, as checking once things have hit master is a bit late...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't have self-hosted runners in a PR environment. Also, all of this should move to a separate repo with added protections on who can edit and call the CI steps on the workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just name it correctly from the start?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be dropped. I.e this file should never have changed its name...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, can we name this correctly from the start?
I've come up with a slightly (much) easier workflow, not too dissimilar from what we currently use for njs (https://github.com/nginx/njs/blob/master/.github/workflows/ci.yml and https://github.com/nginx/ci-self-hosted/blob/main/.github/workflows/njs-buildbot.yml). I'll follow up with a PR for that soon-ish. |
Thanks for the heads up; closing in lieu of the new approach. |
Hello,
This pull request utilizes nginx self hosted runners to build unit and it's modules on a variety of distros and two archs (for the lack of others for now).
The proposed CI creates 252 jobs which can be a bit excessive and may require some optimisations: some jobs don't really do what they should do just skipping test. It's mostly true for python, java and php where each distro has a number of version available and I was too lazy to get a proper configuration for what plays where.
I also replicated configuration management system by creating
setup.sh
which installs packages from a number of lists. And I created those per-OS list. The list is a bit excessive and replicates current buildbot workers. So there is some room for optimisations there as well.The existing CI is left active for pull requests as it provides relatively fast way of checking code for obvious problems.