Skip to content

Conversation

kevinGC
Copy link
Contributor

@kevinGC kevinGC commented Aug 13, 2025

It's only called in one place, and is effectively a method on conn.

Part of #8510.

Note that this PR includes #8509. GitHub doesn't support proper commit chains / stacked PRs, so I'm doing this in several PRs with some (annoyingly) redundant commits. Let me know if this isn't a good workflow for you and I'll change things up.

RELEASE NOTES: N/A

Copy link

codecov bot commented Aug 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.85%. Comparing base (55e8b90) to head (9efdb0d).
⚠️ Report is 20 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8511      +/-   ##
==========================================
- Coverage   82.40%   81.85%   -0.56%     
==========================================
  Files         414      413       -1     
  Lines       40531    40520      -11     
==========================================
- Hits        33399    33167     -232     
- Misses       5770     5982     +212     
- Partials     1362     1371       +9     
Files with missing lines Coverage Δ
credentials/alts/internal/conn/common.go 100.00% <ø> (ø)
credentials/alts/internal/conn/record.go 81.00% <100.00%> (+2.80%) ⬆️

... and 43 files with indirect coverage changes

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

@arjan-bal
Copy link
Contributor

Note that this PR includes #8509. GitHub doesn't support proper commit chains / stacked PRs, so I'm doing this in several PRs with some (annoyingly) redundant commits. Let me know if this isn't a good workflow for you and I'll change things up.

@kevinGC, it would be great if you could rebase this branch off of master so both PRs can be merged independently. We shouldn't need Go 1.24 for the changes in this PR.

Copy link
Contributor

@arjan-bal arjan-bal left a comment

Choose a reason for hiding this comment

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

LGTM, with request to remove the Go 1.24 changes. Adding a reviewer from the security team.

@arjan-bal arjan-bal requested a review from gtcooke94 August 14, 2025 07:12
@arjan-bal arjan-bal added the Type: Internal Cleanup Refactors, etc label Aug 14, 2025
@arjan-bal arjan-bal added this to the 1.76 Release milestone Aug 14, 2025
@gtcooke94
Copy link
Contributor

I think the easiest way for reviewers to diff is to just get the prior PRs merged and then this will diff nicely against master

The other option is to make a PR on your fork against the other PR branch if you want us to see the diff right now, then eventually open a PR later

// Frame is not complete yet.
return nil, nil
}
p.nextFrame = b[MsgLenFieldSize+length:]
Copy link
Contributor

Choose a reason for hiding this comment

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

At a glance I personally prefer the previous style of returning nextFrame and not modifying state in this method, I think it makes it easier to follow and test. Do we have a compelling reason to swap it to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to leave as-is or revert back to the old version. It just seemed odd to have a function in common.go that's only called in 1 place and always called s.t. it modifies the same bit of struct state.

Happy with whatever the gRPC team wants -- this was more of an implementation detail on the way to my other changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Deferring to the @gtcooke94 since the security team owns this package. Closing the PR.

It's only called in one place, and is effectively a method on conn.

Part of grpc#8510.
@kevinGC
Copy link
Contributor Author

kevinGC commented Aug 14, 2025

Rebased. Looks like there're differing opinions on whether this is change is worth making -- I'm cool with keeping or dropping it.

@arjan-bal arjan-bal closed this Aug 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants