Skip to content

fix: for Target isActive use net.Dial instead #17251

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
Merged
merged 8 commits into from
May 25, 2023
Merged

fix: for Target isActive use net.Dial instead #17251

merged 8 commits into from
May 25, 2023

Conversation

@jiuker
Copy link
Contributor

@jiuker jiuker commented May 20, 2023 •
edited
Loading

Description

Target isActive should use StatusCode==200

Motivation and Context

How to test this PR?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Optimization (provides speedup with no functional changes)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Fixes a regression (If yes, please add commit-id or PR # here)
  • Unit tests added/updated
  • Internal documentation updated
  • Create a documentation update request here

@jiuker
fix:Target isActive should use StatusCode==200
099fdd2 
Target isActive should use StatusCode==200
@jiuker
Copy link
Contributor Author

jiuker commented May 20, 2023 •
edited
Loading

This BUG is found when I reproduct #17147

shtripat
shtripat approved these changes May 22, 2023
View reviewed changes
Copy link
Contributor

@shtripat shtripat left a comment

Choose a reason for hiding this comment

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

LGTM. Tested and verified

@jiuker jiuker requested a review from shtripat May 22, 2023 06:28
@jiuker
Copy link
Contributor Author

jiuker commented May 22, 2023

@shtripat I changed the order to prevent the body of the resp from not being closed.Review again plz

klauspost
klauspost reviewed May 22, 2023
View reviewed changes
@jiuker jiuker requested a review from klauspost May 22, 2023 08:48
@jiuker jiuker changed the title fix:Target isActive should use StatusCode==200 fix:Target isActive should use StatusCode<400 May 22, 2023
klauspost
klauspost approved these changes May 22, 2023
View reviewed changes
Copy link
Contributor

@klauspost klauspost left a comment

Choose a reason for hiding this comment

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

LGTM

Praveenrajmani
Praveenrajmani reviewed May 23, 2023
View reviewed changes
@jiuker jiuker closed this May 23, 2023
@klauspost klauspost reopened this May 23, 2023
@klauspost
Copy link
Contributor

@jiuker I asked Harsha to make the decision so no need to close yet.

@jiuker
Copy link
Contributor Author

jiuker commented May 23, 2023

asked Harsha to make the decision so no need to close yet.

Ok.

harshavardhana
harshavardhana requested changes May 25, 2023
View reviewed changes
@jiuker jiuker changed the title fix:Target isActive should use StatusCode<400 fix:Target isActive use net.Dail instead May 25, 2023
@harshavardhana
Copy link
Member

PTAL @shtripat

shtripat
shtripat approved these changes May 25, 2023
View reviewed changes
Copy link
Contributor

@shtripat shtripat left a comment

Choose a reason for hiding this comment

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

lgtm

@jiuker
Copy link
Contributor Author

jiuker commented May 25, 2023

@klauspost pipeline failed.It related to this pr?

@klauspost klauspost changed the title fix:Target isActive use net.Dail instead fix:Target isActive use net.Dial instead May 25, 2023
@klauspost
Copy link
Contributor

I don't really see how webhooks could affect this. I'll rerun it.

@harshavardhana harshavardhana changed the title fix:Target isActive use net.Dial instead fix: for Target isActive use net.Dial instead May 25, 2023
@harshavardhana harshavardhana merged commit 443250d into minio:master May 25, 2023
@bh4t bh4t added the bugfix label May 31, 2023
@jiuker jiuker deleted the patch-102 branch February 26, 2024 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Praveenrajmani Praveenrajmani Praveenrajmani approved these changes

@klauspost klauspost klauspost approved these changes

@shtripat shtripat shtripat approved these changes

@harshavardhana harshavardhana Awaiting requested review from harshavardhana

Assignees

No one assigned

Labels

bugfix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jiuker @klauspost @harshavardhana @Praveenrajmani @shtripat @bh4t