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

removing Roots deprecated Subjects field in tests #6907

Merged
merged 4 commits into from Jan 16, 2024

Conversation

zedGGs
Copy link
Contributor

@zedGGs zedGGs commented Jan 4, 2024

Regarding this issue, #6782

one of the goals was to remove usage of deprecated field Subjects, which is not being returned by SystemCertPool anymore, more details here https://pkg.go.dev/crypto/x509#CertPool.Subjects
also 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

Copy link

codecov bot commented Jan 4, 2024

Codecov Report

Merging #6907 (b11a0bd) into master (953d12a) will decrease coverage by 0.19%.
The diff coverage is n/a.

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     

see 19 files with indirect coverage changes

@arvindbr8 arvindbr8 added the Type: Internal Cleanup Refactors, etc label Jan 5, 2024
@arvindbr8 arvindbr8 added this to the 1.61 Release milestone Jan 5, 2024
Copy link
Member

@arvindbr8 arvindbr8 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 the PR. We need another maintainer's review before we can check this in. cc: @ginayeh could you please take a look at this?

@zedGGs
Copy link
Contributor Author

zedGGs commented Jan 5, 2024

Heey @arvindbr8 , thanks for reply, as I see everyone is on vacation and you are in charge now hehe, cheers !
btw I can also update comments in tests

@zedGGs
Copy link
Contributor Author

zedGGs commented Jan 8, 2024

@arvindbr8 , I had just removed unneeded comments, where Subjects are mentioned, that are not used anymore
and cmp that is not used anymore,

I suppose there's no need for comments since its obvious from Equal what are we doing, similar to code above in same files, where Equal is only used without additional explanation,

I can still update and change this if you want

Copy link
Member

@arvindbr8 arvindbr8 left a 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?

@arvindbr8 arvindbr8 linked an issue Jan 8, 2024 that may be closed by this pull request
@arvindbr8
Copy link
Member

Marking this PR as fixing #6782 since the other half of the issue was already fixed by you @zedGGs

@zedGGs
Copy link
Contributor Author

zedGGs commented Jan 10, 2024

@arvindbr8 I have updated pr, but not sure is this still okey ? since vet-proto is not passing in pipeline,

btw I added small update on vet-proto

this: 872cc34,
was not applied here: https://github.com/grpc/grpc-go/blob/master/binarylog/grpc_binarylog_v1/binarylog.pb.go#L433

@dfawley dfawley assigned arvindbr8 and unassigned zedGGs Jan 10, 2024
Copy link
Member

@arvindbr8 arvindbr8 left a 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 :)

Copy link
Member

Choose a reason for hiding this comment

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

Please roll this back

Copy link
Contributor Author

@zedGGs zedGGs Jan 12, 2024

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

@arvindbr8 arvindbr8 assigned zedGGs and unassigned arvindbr8 Jan 10, 2024
@zedGGs zedGGs force-pushed the remove-deprecated-root-subjects branch 2 times, most recently from ffc5841 to a4630e9 Compare January 12, 2024 14:13
@zedGGs zedGGs force-pushed the remove-deprecated-root-subjects branch from a4630e9 to ef10507 Compare January 12, 2024 14:34
@arvindbr8
Copy link
Member

Thanks, this looks good to me. cc'ing @ginayeh for a second pass

@ginayeh ginayeh merged commit 8b455de into grpc:master Jan 16, 2024
14 checks passed
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.

Remove use of deprecated functions
3 participants