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

Add dependabot and upgrade golang and dependency versions #3

Merged
merged 2 commits into from
Jan 9, 2025

Conversation

djdongjin
Copy link
Member

@djdongjin djdongjin commented Dec 26, 2024

  1. Add dependabot CI.
  2. Update golang to 1.21.

@djdongjin djdongjin force-pushed the add-dependabot-ci branch 3 times, most recently from 3c46bb8 to 9ba4531 Compare December 26, 2024 23:42
@djdongjin
Copy link
Member Author

Hi @klihub could you PTAL this PR? thank you!

@djdongjin djdongjin changed the title Upgrade golang and dependency versions Add dependabot, and upgrade golang and dependency versions Dec 28, 2024
@djdongjin djdongjin changed the title Add dependabot, and upgrade golang and dependency versions Add dependabot and upgrade golang and dependency versions Dec 28, 2024
@djdongjin djdongjin force-pushed the add-dependabot-ci branch 9 times, most recently from 5cffa2a to 02847f8 Compare December 30, 2024 16:49
Comment on lines +8 to +11
go.opentelemetry.io/otel v1.29.0
go.opentelemetry.io/otel/metric v1.29.0
go.opentelemetry.io/otel/sdk v1.29.0
go.opentelemetry.io/otel/trace v1.29.0
Copy link
Member Author

@djdongjin djdongjin Dec 31, 2024

Choose a reason for hiding this comment

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

Since this lib is mainly related to otel, I think we should use relatively newer version of otel libs?

I upgrade to 1.29.0 since that's the latest one requiring go 1.21.
1.30.0 - 1.33.0 all use go 1.22 at least.

Please let me know if the current change is okay, or we should just go to otel 1.33.0 1.31.0 and go 1.22 (same as containerd) .

Copy link
Member

Choose a reason for hiding this comment

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

I think regarding otel, we should make sure that we don't try to force a newer minimal version than what is in use in containerd or in CRI-O.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think regarding otel, we should make sure that we don't try to force a newer minimal version than what is in use in containerd or in CRI-O.

yeah make sense. (and we should apply this to all/most deps? i.e., deps should be lower/equal than the ones in cri-o/containerd).

for otel, containerd is using 1.31.0, cri-o 1.33.0, so bumping to 1.29.0 should be okay?

https://github.com/containerd/containerd/blob/f38df4934824c1de3e1c4092017929f4cece9592/go.mod#L69-L74

https://github.com/cri-o/cri-o/blob/9165374efe1323244b7a18dc3620f8ef36088fa7/go.mod#L66-L69

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, in 2.x we have 1.31.0 but then on the 1.7.x branch we have 1.21.0. But, I'm not sure if we should care about that... Currently otelttrpc is not used on the 1.7 branch.

Copy link
Member

Choose a reason for hiding this comment

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

yeah make sense. (and we should apply this to all/most deps? i.e., deps should be lower/equal than the ones in cri-o/containerd).

Yes, I think we should try to use this for all deps.

Copy link
Member Author

Choose a reason for hiding this comment

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

But, I'm not sure if we should care about that... Currently otelttrpc is not used on the 1.7 branch.

Yeah I also noticed that. I think we only apply important bug/security fixes for backporting to active branches, so it shouldn't matter? 🤔 but please let me know if there are other concerns.

Yes, I think we should try to use this for all deps.

Agreed. Also on the dependabot change in this PR. I think it's still useful for detecting security issues in deps, we don't need to proactively merge those PRs and we can reduce number of PRs generated. But let me know if we want it or not for go deps :)

Signed-off-by: Jin Dong <djdongjin95@gmail.com>
Signed-off-by: Jin Dong <djdongjin95@gmail.com>
@klihub klihub self-requested a review January 9, 2025 16:39
@djdongjin
Copy link
Member Author

fyi @kzys @dmcgowan I've rebased and resolved conflict. PTAL, thank you!

@dmcgowan dmcgowan merged commit fa91caf into containerd:main Jan 9, 2025
7 checks passed
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