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

advancedtls: fix docstring for VerificationResults #7168

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

arjan-bal
Copy link
Contributor

@arjan-bal arjan-bal commented Apr 24, 2024

The static checks enabled in #7155 are failing due #7140 being merged around the same time. #7140 removed the doc string for VerificationResults and replaced it with a deprecation note. As a result the static check failed with the following error:

Error: advancedtls.go:70:1: comment on exported type VerificationResults should be of the form "VerificationResults ..." (with optional leading article) (ST1021)

Example failure: https://github.com/grpc/grpc-go/actions/runs/8811877128/job/24186556563?pr=7163

This change brings back the first line of the doc string to satisfy the test.

RELEASE NOTES: none

@arjan-bal arjan-bal added the Type: Internal Cleanup Refactors, etc label Apr 24, 2024
@arjan-bal arjan-bal added this to the 1.64 Release milestone Apr 24, 2024
@arjan-bal arjan-bal force-pushed the fix-static-check branch 2 times, most recently from 5d09a28 to 3f9f427 Compare April 24, 2024 09:38
@arjan-bal arjan-bal self-assigned this Apr 24, 2024
@@ -67,6 +67,9 @@ type VerificationFuncParams = HandshakeVerificationInfo
// future to include more information.
type PostHandshakeVerificationResults struct{}

// VerificationResults contains the information about results of
// CustomVerificationFunc.
Copy link
Contributor

Choose a reason for hiding this comment

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

is this moved to new line by auto formatter?

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 think so, it was the same earlier:

// VerificationResults contains the information about results of
// CustomVerificationFunc.
// VerificationResults is an empty struct for now. It may be extended in the

Copy link
Contributor

Choose a reason for hiding this comment

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

So what's the fix here, how come tests are passed now?

Copy link
Contributor

@purnesh42H purnesh42H 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 for quick fix!

@arjan-bal arjan-bal assigned aranjans and unassigned arjan-bal Apr 24, 2024
@aranjans
Copy link
Contributor

I think this fix is also part of this PR https://github.com/grpc/grpc-go/pull/7151/files

@arjan-bal
Copy link
Contributor Author

I think this fix is also part of this PR https://github.com/grpc/grpc-go/pull/7151/files

I see. @gtcooke94, will #7151 be merged soon? If not, should we fix only the lint error to make the tests healthy again?

@arjan-bal arjan-bal assigned gtcooke94 and unassigned aranjans Apr 24, 2024
Copy link
Contributor

@gtcooke94 gtcooke94 left a comment

Choose a reason for hiding this comment

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

No worries just merging this as-is, I'll deal with any git weirdness on #7151 on my own

@arjan-bal
Copy link
Contributor Author

@dfawley PTAL and merge if it looks good. I can't merge since I don't have write access.

@arjan-bal arjan-bal removed their assignment Apr 24, 2024
@dfawley dfawley changed the title Bring back the doc string for VerificationResults to fix static check advancedtls: fix docstring for VerificationResults Apr 24, 2024
@dfawley dfawley merged commit 4e8f9d4 into grpc:master Apr 24, 2024
12 checks passed
@arjan-bal arjan-bal deleted the fix-static-check branch September 4, 2024 20:52
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.

5 participants