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 and vendor various dependencies prior to a new release #7526

Conversation

kwilczynski
Copy link
Member

@kwilczynski kwilczynski commented Nov 30, 2023

What type of PR is this?

/kind dependency-change
/assign kwilczynski

What this PR does / why we need it:

Update various direct build time dependencies prior to a new release. This change also closes several dependency changes that had been open for a while.

Related:

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

See review comments.

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Nov 30, 2023
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2023
@openshift-ci openshift-ci bot added the kind/dependency-change Categorizes issue or PR as related to changing dependencies label Nov 30, 2023
github.com/containerd/ttrpc v1.2.2
github.com/containerd/typeurl v1.0.3-0.20220422153119-7f6e6d160d67
github.com/containernetworking/cni v1.1.2
github.com/containernetworking/plugins v1.3.0
github.com/containers/common v0.55.4
github.com/containers/common v0.57.0
Copy link
Member Author

Choose a reason for hiding this comment

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

This change means that the following Pull Request is no longer needed:

github.com/checkpoint-restore/go-criu/v6 v6.3.0
github.com/container-orchestrated-devices/container-device-interface v0.6.0
github.com/checkpoint-restore/checkpointctl v1.1.0
github.com/checkpoint-restore/go-criu/v7 v7.0.0
Copy link
Member Author

Choose a reason for hiding this comment

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

This change follows the following upstream update:

And, when included, would resolve the following build time error:

panic: proto: file "stats/stats.proto" is already registered
	previously from: "github.com/checkpoint-restore/go-criu/v6/stats"
	currently from:  "github.com/checkpoint-restore/go-criu/v7/stats"

github.com/containers/conmon v2.0.20+incompatible
github.com/containers/conmon-rs v0.6.1
github.com/containers/conmon-rs v0.6.2-0.20230920142715-f5a362044a57
Copy link
Member Author

Choose a reason for hiding this comment

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

This change follows the following upstream change:

And, when included, would resolve the following build time error:

# github.com/containers/conmon-rs/pkg/client
vendor/github.com/containers/conmon-rs/pkg/client/attach.go:232:19: undefined: util.CopyDetachable
vendor/github.com/containers/conmon-rs/pkg/client/attach.go:429:26: undefined: util.ErrDetach

Copy link
Member Author

Choose a reason for hiding this comment

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

To add:

This vendors a specific commit for the time being in anticipation of a future new release of conmon-rs.

@kwilczynski kwilczynski force-pushed the feature/update-various-dependencies branch from 92da412 to 3c7d125 Compare November 30, 2023 11:59
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2023
@kwilczynski kwilczynski force-pushed the feature/update-various-dependencies branch 2 times, most recently from 76db83b to 767f4ce Compare November 30, 2023 12:33
@@ -7,29 +7,28 @@ require (
github.com/Microsoft/go-winio v0.6.1
github.com/blang/semver v3.5.1+incompatible
github.com/blang/semver/v4 v4.0.0
github.com/checkpoint-restore/checkpointctl v0.1.0
github.com/checkpoint-restore/go-criu/v6 v6.3.0
github.com/container-orchestrated-devices/container-device-interface v0.6.0
Copy link
Member Author

Choose a reason for hiding this comment

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

This change follows the following upstream update:

And, when included, would resolve package import issues.

@kwilczynski kwilczynski force-pushed the feature/update-various-dependencies branch 2 times, most recently from 4af7912 to 9a0b54c Compare November 30, 2023 12:45
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

The linter is not happy, though:

  Error: SA1019: otelgrpc.UnaryServerInterceptor is deprecated: Use [NewServerHandler] instead.  (staticcheck)
  Error: SA1019: otelgrpc.StreamServerInterceptor is deprecated: Use [NewServerHandler] instead.  (staticcheck)

@kwilczynski
Copy link
Member Author

The linter is not happy, though:

  Error: SA1019: otelgrpc.UnaryServerInterceptor is deprecated: Use [NewServerHandler] instead.  (staticcheck)
  Error: SA1019: otelgrpc.StreamServerInterceptor is deprecated: Use [NewServerHandler] instead.  (staticcheck)

@saschagrunert, we can fix this later in a follow-up Pull Request.

This might be a larger body of work, and I don't want to mix dependency updates with code refactorings, if possible.

Unless you want this fixed here?

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Merging #7526 (1e15c38) into main (87c53ac) will decrease coverage by 0.01%.
Report is 2 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7526      +/-   ##
==========================================
- Coverage   48.69%   48.68%   -0.01%     
==========================================
  Files         145      145              
  Lines       15926    15926              
==========================================
- Hits         7755     7754       -1     
- Misses       7236     7237       +1     
  Partials      935      935              

@saschagrunert
Copy link
Member

@kwilczynski I'd prefer to fix it within this PR, you can just go for a second commit on top of the dependency changes.

@kwilczynski
Copy link
Member Author

@kwilczynski I'd prefer to fix it within this PR, you can just go for a second commit on top of the dependency changes.

@saschagrunert, OK! Will do.

@kwilczynski kwilczynski force-pushed the feature/update-various-dependencies branch from 9a0b54c to d6ad94e Compare December 4, 2023 10:13
@kwilczynski
Copy link
Member Author

kwilczynski commented Dec 4, 2023

@kwilczynski I'd prefer to fix it within this PR, you can just go for a second commit on top of the dependency changes.

@saschagrunert, OK! Will do.

Done. Completed in cc956e6.

@kwilczynski
Copy link
Member Author

/assign hswong3i

Copy link
Contributor

openshift-ci bot commented Dec 4, 2023

@kwilczynski: GitHub didn't allow me to assign the following users: hswong3i.

Note that only cri-o members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign hswong3i

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/test-infra repository.

@klihub
Copy link
Contributor

klihub commented Dec 4, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2023
@klihub
Copy link
Contributor

klihub commented Dec 4, 2023

/lgtm cancel

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2023
@kwilczynski kwilczynski force-pushed the feature/update-various-dependencies branch 3 times, most recently from 5576df6 to 8dd33d4 Compare December 4, 2023 17:05
kwilczynski and others added 4 commits December 5, 2023 02:11
Signed-off-by: Krzysztof Wilczyński <kwilczynski@redhat.com>
Signed-off-by: Krzysztof Wilczyński <kwilczynski@redhat.com>
Signed-off-by: Krzysztof Wilczyński <kwilczynski@redhat.com>
Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
@kwilczynski kwilczynski force-pushed the feature/update-various-dependencies branch from 8dd33d4 to 1e15c38 Compare December 4, 2023 17:13
Copy link
Member

@sohankunkerkar sohankunkerkar left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2023
Copy link
Contributor

openshift-ci bot commented Dec 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kwilczynski, saschagrunert, sohankunkerkar

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:
  • OWNERS [kwilczynski,saschagrunert,sohankunkerkar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@haircommander
Copy link
Member

/override ci/prow/ci-e2e-conmonrs

Copy link
Contributor

openshift-ci bot commented Dec 4, 2023

@haircommander: Overrode contexts on behalf of haircommander: ci/prow/ci-e2e-conmonrs

In response to this:

/override ci/prow/ci-e2e-conmonrs

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/test-infra repository.

@haircommander
Copy link
Member

/retest

1 similar comment
@sohankunkerkar
Copy link
Member

/retest

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. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/dependency-change Categorizes issue or PR as related to changing dependencies lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants