Skip to content
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

Open
wants to merge 2 commits into
base: v4.4.1-crio
Choose a base branch
from

Conversation

TomSweeneyRedHat
Copy link
Member

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?

None

@TomSweeneyRedHat TomSweeneyRedHat added the No New Tests Allow PR to proceed without adding regression tests label Jun 8, 2024
@openshift-ci openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 8, 2024
@kwilczynski
Copy link
Member

@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?

@kwilczynski
Copy link
Member

/approve
/lgtm

Copy link
Contributor

openshift-ci bot commented Jun 8, 2024

@kwilczynski: changing LGTM is restricted to collaborators

In response to this:

/approve
/lgtm

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.

Copy link
Contributor

openshift-ci bot commented Jun 8, 2024

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mheon
Copy link
Member

mheon commented Jun 11, 2024

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>
@TomSweeneyRedHat
Copy link
Member Author

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 make complete, add the changes, rebase, and repush. Fingers crossed! Thanks again for the assist Ed, much appreciated.

@TomSweeneyRedHat
Copy link
Member Author

@mheon I'm not surprised, this is bloated, do you want to slap a tag on this?

@mheon mheon added the bloat_approved Approve a PR in which binary file size grows by over 50k label Jun 11, 2024
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>
@TomSweeneyRedHat
Copy link
Member Author

@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?

@cevich
Copy link
Member

cevich commented Jun 13, 2024

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.

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?

@kwilczynski
Copy link
Member

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?

@haircommander
Copy link
Collaborator

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bloat_approved Approve a PR in which binary file size grows by over 50k No New Tests Allow PR to proceed without adding regression tests release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants