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.

Sign up for GitHub

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

Jump to bottom

Add validation callback and tests #4818

Merged
merged 22 commits into from
Jul 16, 2024
Merged

Add validation callback and tests #4818

merged 22 commits into from
Jul 16, 2024

Conversation

neha-bhargava
Copy link
Contributor

Fixes #4462

Changes proposed in this request
Add callback for cert validation
Unit test the callback

Testing
Unit test

Performance impact

Documentation

  • All relevant documentation is updated.

@neha-bhargava neha-bhargava requested a review from a team as a code owner June 19, 2024 21:34
bgavrilMS
bgavrilMS requested changes Jun 24, 2024
View reviewed changes
Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

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

Current update creates a new HttpClient for each request, which is problematic. Please complete the Lazy approach you started or use a static.

@neha-bhargava neha-bhargava requested a review from bgavrilMS June 27, 2024 19:49
bgavrilMS
bgavrilMS previously approved these changes Jul 9, 2024
View reviewed changes
Base automatically changed from gladjohn/updatedHttpManager to main July 11, 2024 17:30
@neha-bhargava neha-bhargava dismissed bgavrilMS’s stale review July 11, 2024 17:30

The base branch was changed.

gladjohn
gladjohn approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@gladjohn gladjohn left a comment

Choose a reason for hiding this comment

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

looks Neha, were you able to validate this on a service fabric environment?

@neha-bhargava
Copy link
Contributor Author

looks Neha, were you able to validate this on a service fabric environment?

No, since the setup was not easy it was decided to rely on the unit tests instead

@neha-bhargava neha-bhargava merged commit 9049fe9 into main Jul 16, 2024
6 checks passed
@neha-bhargava neha-bhargava deleted the nebharg/sfUpdates branch July 16, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gladjohn gladjohn gladjohn approved these changes

@bgavrilMS bgavrilMS bgavrilMS left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Service Fabric MI Auth doesn't validate server cert.
4 participants
@neha-bhargava @bgavrilMS @gladjohn @GladwinJohnson