-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
removing Roots deprecated Subjects field in tests #6907
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #6907 +/- ##
==========================================
- Coverage 83.72% 83.53% -0.19%
==========================================
Files 287 287
Lines 30835 30835
==========================================
- Hits 25816 25759 -57
- Misses 3964 4010 +46
- Partials 1055 1066 +11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the PR. We need another maintainer's review before we can check this in. cc: @ginayeh could you please take a look at this?
Heey @arvindbr8 , thanks for reply, as I see everyone is on vacation and you are in charge now hehe, cheers ! |
@arvindbr8 , I had just removed unneeded comments, where I suppose there's no need for comments since its obvious from I can still update and change this if you want |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! The updates made to the comments LGTM
However, I think we remove corresponding ignores of those usages in vet.sh. Could you please append your PR with that?
Marking this PR as fixing #6782 since the other half of the issue was already fixed by you @zedGGs |
@arvindbr8 I have updated pr, but not sure is this still okey ? since btw I added small update on this: 872cc34, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the changes looks good to me. Please address that one comment about the pb.go
file
btw I added small update on vet-proto
We should not be doing that as part of this change. I have actually merged a change to update the pb.go
file. Please rollback that file and rebase from master -- and vet-proto should be happy :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please roll this back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for the reivew @arvindbr8 , I have updated pr but tests seems not to pass Im not sure is that because of flakines with xds tests : https://github.com/grpc/grpc-go/actions/runs/7498064395/job/20412613973
I apologise, I have managed somehow to have push from wrong account, I
if this matters I can add separated pr for this,
but I have fixed that too now with proper rebase and commits update so everything is good
ffc5841
to
a4630e9
Compare
a4630e9
to
ef10507
Compare
Thanks, this looks good to me. cc'ing @ginayeh for a second pass |
Regarding this issue, #6782
one of the goals was to remove usage of deprecated field
Subjects
, which is not being returned bySystemCertPool
anymore, more details here https://pkg.go.dev/crypto/x509#CertPool.Subjectsalso I have updated these tests so we compare got and wanted CertPool structs, by using https://pkg.go.dev/crypto/x509#CertPool.Equal
RELEASE NOTES: none