-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[v4.4.1-crio] Bump c/common to v0.52.0 #22944
base: v4.4.1-crio
Are you sure you want to change the base?
[v4.4.1-crio] Bump c/common to v0.52.0 #22944
Conversation
@TomSweeneyRedHat, thank you for tending to this. Appreciated! It seems that some of the checks are failing in the post-steps. I am not sure why, though. Some older script versions, perhaps? |
/approve |
@kwilczynski: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kwilczynski, TomSweeneyRedHat The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Build failures are legitimate. Completions errors? |
As the title says. This replaces containers#22355. I'm not sure how I managed, but the original PR: containers#22355, I accidentally pushed into the v4.4.1-crio branch as a merge commit. I've reverted that, and redid the vendor for c/common 0.52.0 This dragged in c/image v5.25.0, and I also needed to bump image builder as the version of Docker that also got dragged in dropped the LCOWSupported functionality and c/storage by that time had taken it over. So long story short, a lot more changes than I'm entirely comfortable with. @kwilczynski PTAL. Sorry about the very slow response on this, we had to get some CVE fixes first, and in so doing, discovered the CI in this branch was pretty badly broken and had to fix that . Addresses: https://issues.redhat.com/browse/OCPBUGS-32144 Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>
a120f0e
to
802ddb3
Compare
Repushed, with my learning for the day. @edsantiago and his eagle eye spotted the issue in the voluminous log. Given the change in the cobra that was vendored in, I needed to |
@mheon I'm not surprised, this is bloated, do you want to slap a tag on this? |
b2e735b
to
37c0810
Compare
Three calls for lint for the below function: ``` //nolint:staticcheck g := generate.NewFromSpec(c.config.Spec) ``` we failing with this error in the F37 builds: libpod/container_internal_common.go:167:2: directive `//nolint:staticcheck` is unused for linter "staticcheck" (nolintlint) //nolint:staticcheck ^ As the nolint lines have been removed from the main tree for the calls to `generate.NewFromSpec()`, I'm going to try here as well. Also, the LastCap() function has since moved from github.com/opencontainers/runtime-tools/validate to github.com/opencontainers/runtime-tools/validate/capabilities. Change that as well. Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>
37c0810
to
7b7b850
Compare
@cevich, what are your thoughts on this one? I had to turn off some lint checks and, more importantly, move to a new capabilities check. I'm not sure if that last change or one of the many new files is asking for a Golang bump. Do you have any thoughts on the next steps here? |
Bumping golang on an old release branch is always a big risk for overall CI-health as you and I have been through countless times at this point. I'm assuming you tried the suggested vendoring fix. Is that what is suggesting the golang bump? |
I think, we are reaching a point where we need to decide whether we need this branch and the bump. This has become a bit of a headache to maintain. What do you think, @haircommander? |
I don't necessarily think we need fedora rpm builds for these branches. If we continue to package this branch in openshift we can use that build process to catch issues, as these builds will never make it into fedora (or any real version of rhel either). what do folks think? |
As the title says. This replaces
#22355.
I'm not sure how I managed, but I accidentally pushed the original PR, #22355, into the v4.4.1-crio branch as a merge commit.
I've reverted that, and redid the vendor for c/common 0.52.0
This dragged in c/image v5.25.0, and I also needed to bump image builder as the version of Docker that also got dragged in dropped the LCOWSupported functionality and c/storage by that time had taken it over.
So long story short, a lot more changes than I'm entirely comfortable with.
@kwilczynski PTAL. Sorry about the very slow response on this, we had to get some CVE fixes first, and in so doing, discovered the CI in this branch was pretty badly broken and had to fix that .
Addresses: https://issues.redhat.com/browse/OCPBUGS-32144
Does this PR introduce a user-facing change?