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

Feature/async refactor #30

Merged
merged 2 commits into from
Oct 23, 2024
Merged

Feature/async refactor #30

merged 2 commits into from
Oct 23, 2024

Conversation

ikrivosheev
Copy link
Contributor

@ikrivosheev ikrivosheev commented Oct 22, 2024 •
edited by michaelklishin
Loading

This is a continuation to #29.

Closes #27.

@michaelklishin
Copy link
Owner

Looks like #29 was extracted from this PR with some further changes. This needs a rebase.

(I'm assuming that it's too early to evaluate this PR further since it is currently a draft)

@ikrivosheev ikrivosheev force-pushed the feature/async_refactor branch from ba9ccc4 to 8f8813e Compare October 23, 2024 06:41
@ikrivosheev ikrivosheev marked this pull request as ready for review October 23, 2024 06:42
@ikrivosheev
Copy link
Contributor Author

@michaelklishin well, I've done with the PR.

michaelklishin
michaelklishin requested changes Oct 23, 2024
View reviewed changes
Copy link
Owner

@michaelklishin michaelklishin left a comment

Choose a reason for hiding this comment

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

I like where this is headed. We need to correct one error message before this can be merged, though, as it is overly specific.

src/error.rs
ClientErrorResponse(StatusCode, Respone),
#[error("API responded with a server error: status code of {0}")]
ServerErrorResponse(StatusCode, Respone),
#[error("Health check failed: resource alarms are in effect")]
Copy link
Owner

@michaelklishin michaelklishin Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not all health checks are related to resource alarms but IIRC all of them use the same HTTP status code for failures. So this message must be shortened but the enum value makes sense.

src/error.rs
HealthCheckFailed(responses::HealthCheckFailureDetails),
#[error("Could not find the requested resource")]
NotFound(),
#[error("Can't delete a binding: multiple matching bindings found")]
Copy link
Owner

Choose a reason for hiding this comment

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

I've been contributing to RabbitMQ and/or clients libraries since 2010 and I have never seen this error. But it makes some sense, the only way to differentiate between multiple bindings between a source exchange and its destination is via their properties and they were not provided, returning an error is an option.

michaelklishin added a commit that referenced this pull request Oct 23, 2024
@michaelklishin
Wording #30
380d815
@michaelklishin michaelklishin merged commit d790145 into michaelklishin:main Oct 23, 2024
2 checks passed
@michaelklishin
Copy link
Owner

I figured I might as well update the messages myself.

@michaelklishin michaelklishin added this to the 0.9.0 milestone Oct 23, 2024
@ikrivosheev ikrivosheev deleted the feature/async_refactor branch October 23, 2024 19:35
michaelklishin added a commit that referenced this pull request Nov 22, 2024
@michaelklishin
Do not assume that HTTP API prefix is always "api"
b163086 
It can be overridden and management plugin's own
test suite runs most tests both with the default
and an overridden prefix.

References #29, #30, #31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelklishin michaelklishin michaelklishin requested changes

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

Successfully merging this pull request may close these issues.

Support async interface
2 participants
@ikrivosheev @michaelklishin