Skip to content

[2025-04-24] Manual Dependency Bump #19792

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

Merged
merged 2 commits into from
Apr 27, 2025
Merged

Conversation

joshjms
Copy link
Member

@joshjms joshjms commented Apr 24, 2025

@k8s-ci-robot
Copy link

Hi @joshjms. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@henrybear327
Copy link
Contributor

/ok-to-test

@joshjms
Copy link
Member Author

joshjms commented Apr 24, 2025

Thanks @henrybear327 I'll update this PR for any future relevant dependabot bumps. I'll set it as ready to merge at the end of the week right?

@joshjms joshjms marked this pull request as ready for review April 24, 2025 07:57
@joshjms
Copy link
Member Author

joshjms commented Apr 24, 2025

cc @ivanvc

@joshjms
Copy link
Member Author

joshjms commented Apr 24, 2025

For v1.71.x, Resolver has an attribute CC. In v1.72.0, the attribute CC is made private (cc) with the addition of a public method to access it r.CC().

func (r *Resolver) CC() resolver.ClientConn {
	r.mu.Lock()
	defer r.mu.Unlock()
	if r.cc == nil {
		panic("Manual resolver instance has not yet been built.")
	}
	return r.cc
}

Refer to grpc/grpc-go@5668c66#diff-ab6658976ec936cefd2c07700b5ed0e2dfe061ad6a5185479a4cd4b77b45e415R121

@joshjms joshjms force-pushed the deps_mgmt_21_04_25 branch from 12722dc to 3562b79 Compare April 24, 2025 08:34
@joshjms
Copy link
Member Author

joshjms commented Apr 24, 2025

Changing from .CC to .CC() causes the panic to happen:

panic: Manual resolver instance has not yet been built.

@joshjms joshjms force-pushed the deps_mgmt_21_04_25 branch from 5e9c94b to fa38b54 Compare April 24, 2025 09:21
@joshjms
Copy link
Member Author

joshjms commented Apr 24, 2025

/retest

@joshjms
Copy link
Member Author

joshjms commented Apr 24, 2025

To handle this I just use a defer recover to return cc = nil when receiving the panic

@ivanvc
Copy link
Member

ivanvc commented Apr 24, 2025

Thanks @henrybear327 I'll update this PR for any future relevant dependabot bumps. I'll set it as ready to merge at the end of the week right?

@joshjms, dependabot runs on Mondays. So, there's no need to wait until the end of the week. We can merge as soon as you have the pull request ready.

@joshjms
Copy link
Member Author

joshjms commented Apr 24, 2025

Well noted, thanks for the clarification.

@ahrtr
Copy link
Member

ahrtr commented Apr 25, 2025

For v1.71.x, Resolver has an attribute CC. In v1.72.0, the attribute CC is made private (cc) with the addition of a public method to access it r.CC().

func (r *Resolver) CC() resolver.ClientConn {
	r.mu.Lock()
	defer r.mu.Unlock()
	if r.cc == nil {
		panic("Manual resolver instance has not yet been built.")
	}
	return r.cc
}

Refer to grpc/grpc-go@5668c66#diff-ab6658976ec936cefd2c07700b5ed0e2dfe061ad6a5185479a4cd4b77b45e415R121

We really don't want to see surprise/breaking change. At least grpc-go should go through a deprecation process for any breaking change. @dfawley But it isn't a big problem for this case, because it doesn't affect any etcd's users.

Next time when we bump grpc-go version for the stable releases, we will have to make similar change.

@ahrtr
Copy link
Member

ahrtr commented Apr 25, 2025

/test pull-etcd-coverage-report

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

thx @joshjms

Copy link

codecov bot commented Apr 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.88%. Comparing base (8f933a5) to head (fa38b54).
Report is 16 commits behind head on main.

Additional details and impacted files
Files with missing lines Coverage Δ
client/v3/internal/resolver/resolver.go 87.50% <100.00%> (+2.31%) ⬆️

... and 18 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19792      +/-   ##
==========================================
+ Coverage   68.81%   68.88%   +0.07%     
==========================================
  Files         421      421              
  Lines       35863    35868       +5     
==========================================
+ Hits        24678    24708      +30     
+ Misses       9754     9732      -22     
+ Partials     1431     1428       -3     

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f933a5...fa38b54. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@ivanvc ivanvc left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @joshjms.

I would praise for squashing commits because the first commit breaks the functionality. And if we need to cherry-pick from @ahrtr's comment:

Next time when we bump grpc-go version for the stable releases, we will have to make similar change.

It will be easier to cherry-pick a single commit.

But it's not a big deal, we can merge it as-is.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, ivanvc, joshjms

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

@joshjms
Copy link
Member Author

joshjms commented Apr 26, 2025

I would praise for squashing commits because the first commit breaks the functionality.

I'll take note of this next time. Thanks a lot!

@ahrtr ahrtr merged commit da2837c into etcd-io:main Apr 27, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants