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

update dependencies to recent versions #51

Closed
wants to merge 1 commit into from

Conversation

bart0sh
Copy link
Contributor

@bart0sh bart0sh commented Mar 10, 2022

No description provided.

@bart0sh bart0sh force-pushed the PR0012-update-go-mod branch 2 times, most recently from 13e9069 to a369587 Compare March 10, 2022 08:59
@bart0sh bart0sh requested review from elezar, kad and klihub March 10, 2022 09:00
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
Copy link
Contributor

@klihub klihub left a comment

Choose a reason for hiding this comment

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

As @elezar correctly pointed out, we can't update selinux to that version until the related problems (unexplained OpenShift CI kubernetes cluster bootstrapping failures) are solved and this same change is done first in CRI-O.

@elezar
Copy link
Contributor

elezar commented Mar 11, 2022

As @elezar correctly pointed out, we can't update selinux to that version until the related problems (unexplained OpenShift CI kubernetes cluster bootstrapping failures) are solved and this same change is done first in CRI-O.

We could update it, but that would mean that we may run into similar problems when updating the dependency in the crio codebase again.

@elezar
Copy link
Contributor

elezar commented Mar 11, 2022

As soon as @klihub is happy with this, I'm good to merge.

@elezar elezar self-requested a review March 11, 2022 15:49
@bart0sh
Copy link
Contributor Author

bart0sh commented Mar 15, 2022

@klihub I've updated this PR according to your suggestions. Please review again and merge if you're ok with it.

@klihub
Copy link
Contributor

klihub commented Mar 15, 2022

I can't get this working. Unless I revert the runc update back to 1.0.3, go insists updating selinux to 1.10.0.
@bart0sh @elezar Doesn't this PR exhibit the same behavior for you guys ?

@bart0sh
Copy link
Contributor Author

bart0sh commented Mar 15, 2022

@klihub yep, I've mentioned it in this comment

We should probably drop this PR. At least for now.

Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>
@bart0sh
Copy link
Contributor Author

bart0sh commented Mar 15, 2022

@elezar @klihub I've reworked this PR to the state where go.mod is stable. please review again.

@@ -6,9 +6,9 @@ require (
github.com/hashicorp/go-multierror v1.1.1
github.com/opencontainers/runc v1.0.3
github.com/opencontainers/runtime-spec v1.0.3-0.20210326190908-1c3f411f0417
github.com/opencontainers/runtime-tools v0.0.0-20190417131837-cd1349b7c47e
github.com/opencontainers/runtime-tools v0.9.0
Copy link
Contributor

@klihub klihub Mar 15, 2022

Choose a reason for hiding this comment

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

This change will break builds. We use AddMultipleProcessEnv() from runtime-tools which is not part of any tagged release, so it requires a SHA1-exact dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should probably stop using AddMultipleProcessEnv and upgrade this dependency. The commit we're using is almost 3 years old. Another option is to drop this PR for now. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to suggest tagging a new version of runtime-tools so that when one references that new tag the necessary commit gets pulled in ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, if this is a show-stopper then we can rework CDI to use a much older version and later switch back to using AddMultipleProcessEnv, if that ever gets covered by a new tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure it will be ever covered by a new tag. I'll drop this PR as it only upgrades cobra. The rest is not possible to update for one or another reason.

@bart0sh bart0sh closed this Mar 15, 2022
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